-
Notifications
You must be signed in to change notification settings - Fork 689
feat: add SkipMetricsTracking property to DbSettings #11515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ public class DbSettings(string name, string path) | |||||||
|
|
||||||||
| public bool DeleteOnStart { get; set; } | ||||||||
| public bool CanDeleteFolder { get; set; } = true; | ||||||||
| public bool SkipMetricsTracking { get; set; } | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low: Missing XML documentation on this public property. The coding style guide (
Suggested change
(Other properties in this file are also undocumented — consistent with the existing pattern, but worth fixing here since this one has non-obvious semantics.) |
||||||||
|
|
||||||||
| public IMergeOperator? MergeOperator { get; set; } | ||||||||
| public Dictionary<string, IMergeOperator>? ColumnsMergeOperators { get; set; } | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,12 +33,17 @@ protected override void Load(ContainerBuilder builder) | |
| DbSettings stateDbSettings = new(GetTitleDbName(DbNames.State), DbNames.State); | ||
| IFileSystem fileSystem = ctx.Resolve<IFileSystem>(); | ||
| IDbFactory dbFactory = ctx.Resolve<IDbFactory>(); | ||
| return new FullPruningDb( | ||
| FullPruningDb db = new( | ||
| stateDbSettings, | ||
| dbFactory is not MemDbFactory | ||
| ? new FullPruningInnerDbFactory(dbFactory, fileSystem, stateDbSettings.DbPath) | ||
| : dbFactory, | ||
| () => Interlocked.Increment(ref Nethermind.Db.Metrics.StateDbInPruningWrites)); | ||
| // Register the outer wrapper so GatherMetric() always reflects the currently active | ||
| // inner DB, even across full-pruning cycles. Inner DBs are excluded from tracking | ||
| // via SkipMetricsTracking (set by FullPruningInnerDbFactory) to avoid stale entries. | ||
|
|
||
| ctx.ResolveOptional<DbMonitoringModule.DbTracker>()?.AddDb(stateDbSettings.DbName, db); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low: |
||
| return db; | ||
| }) | ||
|
|
||
| .AddSingleton<INodeStorageFactory>(ctx => | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low: This test verifies the
SkipMetricsTrackingflag mechanism, but there's no test for the actual bug scenario: create aFullPruningDb, trigger a pruning cycle (viaTryStartPruning→Commit), then assert that the tracker'sGatherMetric()reads from the new inner DB and not the discarded one. The unit test is good, but an integration test would close the loop on correctness.