Skip to content

Commit 9ce18eb

Browse files
committed
fix(engine) #5636: read the WAL stat map once, and keep the PR-time Meterian signal
Round 2 review on #5640. Four of the five points applied, one declined with the reason recorded in the code. 1. collectDatabaseStats() called TransactionManager.getStats() twice per open database - once inside the fold for pagesWritten/bytesWritten and again for logFiles - and that call allocates a HashMap and walks the active WAL pool each time. Since the whole point of the refactor was to stop maintaining two copies of the loop, accumulateMonotonic now returns the map it read. logFiles cannot join the fold itself: it is a current count, not a total, so carrying it across a close would inflate it forever. 3. unregisteringTwiceFoldsTheCountersOnlyOnce asserted exact equality against a JVM-singleton counter. Today surefire runs forkCount=1/reuseForks=true so classes are sequential, but the assertion would go intermittent the day that changes. Bounded instead: a second fold would add at least this database's own QUERIES again. 4. The two new Schema methods are source-incompatible for a third-party implementor. Noted in the release notes. 5. Meterian keeps running on pull_request. continue-on-error already stops it failing the check, so dropping the PR trigger would have thrown away the per-PR SARIF diff for no gain: a newly introduced vulnerable dependency now still surfaces at PR time rather than only once it lands on main. Point 2 (the scrape path has no per-database try/catch while the close path does) is declined, and why is now a comment on collectDatabaseStats: skipping a database whose stat sources are mid-teardown would drop its contribution from one snapshot and restore it on the next, which is a transient DIP - precisely the counter-reset artifact this change exists to eliminate. Letting the read throw surfaces as a failed scrape, which Prometheus handles by carrying the last value forward. A wrong number is worse than a missing one here.
1 parent d670891 commit 9ce18eb

4 files changed

Lines changed: 31 additions & 6 deletions

File tree

‎.github/workflows/meterian.yml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ name: Meterian Scanner workflow
1818
on:
1919
push:
2020
branches: [main]
21+
# PRs are still scanned. What changed is that the result cannot fail the check (continue-on-error below), so a
22+
# newly introduced vulnerable dependency still surfaces its SARIF diff at PR time instead of only after it lands
23+
# on main - the signal is kept, only the blocking red X is dropped.
24+
pull_request:
2125
schedule:
2226
# Daily, so the alert set stays current between main commits.
2327
- cron: "17 4 * * *"

‎docs/release-26.8.1.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -945,6 +945,10 @@ The API shape is what kept inviting the mistake, so the pattern is closed rather
945945
exposes null-returning `getBucketByIdIfExists(int)` and `getBucketByNameIfExists(String)` - named after the
946946
`getFileByIdIfExists(int)` already on the interface - and the throwing forms document that they throw.
947947

948+
Both in-tree implementors (`LocalSchema`, `RemoteSchema`) are updated. Note the two new interface methods are
949+
source-incompatible for anyone implementing `com.arcadedb.schema.Schema` outside the project: such an
950+
implementation needs the two methods added before it compiles against this release.
951+
948952
## Server and cluster status endpoints scope their per-database output to the caller
949953

950954
The routes that enumerate the whole database registry rather than naming one database in the path now reduce

‎engine/src/main/java/com/arcadedb/Profiler.java‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,15 +122,23 @@ public synchronized void unregisterDatabase(final DatabaseInternal database) {
122122
/**
123123
* Sums the per-database counters of every open database on top of the retained baseline of the closed ones.
124124
* Shared by {@link #toJSON()} and {@link #dumpMetrics(PrintStream)} so the two cannot drift apart.
125+
* <p>
126+
* Deliberately NOT guarded per database, unlike the fold in {@link #unregisterDatabase}. Skipping a database whose
127+
* stat sources are mid-teardown would drop its contribution from this one snapshot and restore it on the next -
128+
* a transient DIP, which is exactly the counter-reset artifact this whole change exists to eliminate. Letting the
129+
* read throw instead surfaces as a failed scrape, which Prometheus already handles by carrying the last value
130+
* forward. A wrong number is worse than a missing one here.
125131
*/
126132
private long[] collectDatabaseStats() {
127133
final long[] acc = new long[STATS_COUNT];
128134
System.arraycopy(retainedStats, 0, acc, 0, MONOTONIC_STATS);
129135

130136
for (final DatabaseInternal db : databases) {
131-
accumulateMonotonic(acc, db);
132-
133-
acc[STAT_WAL_TOTAL_FILES] += statOf(db.getTransactionManager().getStats(), "logFiles");
137+
// The WAL map comes back from the fold rather than being re-read: TransactionManager.getStats() allocates a
138+
// fresh HashMap and walks the active WAL pool on every call, and logFiles lives in the same map as the two
139+
// monotonic WAL counters.
140+
final Map<String, Object> walStats = accumulateMonotonic(acc, db);
141+
acc[STAT_WAL_TOTAL_FILES] += statOf(walStats, "logFiles");
134142

135143
final FileManager.FileManagerStats fStats = db.getFileManager().getStats();
136144
acc[STAT_OPEN_FILES] += fStats.totalOpenFiles;
@@ -146,15 +154,20 @@ private long[] collectDatabaseStats() {
146154
* Adds one database's monotonic counters into {@code acc}. Deliberately touches only the two stat sources that stay
147155
* readable after a close ({@link DatabaseInternal#getStats()} reads a plain counter holder, and
148156
* {@code TransactionManager.getStats()} guards a retired WAL pool), so {@link #unregisterDatabase} can reuse it.
157+
*
158+
* @return the WAL stat map it read, so a caller that also needs the instantaneous {@code logFiles} count out of it
159+
* does not pay for a second {@code TransactionManager.getStats()}. {@code logFiles} cannot join the fold
160+
* itself - it is a current count, not a total, so carrying it across a close would inflate it forever.
149161
*/
150-
private void accumulateMonotonic(final long[] acc, final DatabaseInternal db) {
162+
private Map<String, Object> accumulateMonotonic(final long[] acc, final DatabaseInternal db) {
151163
final Map<String, Object> dbStats = db.getStats();
152164
for (int i = 0; i < DB_STAT_KEYS.length; i++)
153165
acc[i] += statOf(dbStats, DB_STAT_KEYS[i]);
154166

155167
final Map<String, Object> walStats = db.getTransactionManager().getStats();
156168
acc[STAT_WAL_PAGES_WRITTEN] += statOf(walStats, "pagesWritten");
157169
acc[STAT_WAL_BYTES_WRITTEN] += statOf(walStats, "bytesWritten");
170+
return walStats;
158171
}
159172

160173
/**

‎engine/src/test/java/com/arcadedb/Issue5636ProfilerMonotonicTest.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import com.arcadedb.database.Database;
2222
import com.arcadedb.database.DatabaseFactory;
23+
import com.arcadedb.database.DatabaseInternal;
2324
import com.arcadedb.serializer.json.JSONObject;
2425
import com.arcadedb.utility.FileUtils;
2526
import org.junit.jupiter.api.AfterEach;
@@ -113,9 +114,12 @@ void unregisteringTwiceFoldsTheCountersOnlyOnce() {
113114
final long afterFirst = profilerCount("queries");
114115

115116
// The database is already gone from the registry; a second unregister must be a no-op, not a second fold.
116-
Profiler.INSTANCE.unregisterDatabase((com.arcadedb.database.DatabaseInternal) db);
117+
Profiler.INSTANCE.unregisterDatabase((DatabaseInternal) db);
118+
// Bounded rather than exact: a second fold would add at least this database's own QUERIES again, while
119+
// anything else sharing the reused surefire fork can only add a few. Exact equality would go intermittent
120+
// the day the suite runs test classes concurrently.
117121
assertThat(profilerCount("queries")).as("a repeated unregister must not fold the same counters in again")
118-
.isEqualTo(afterFirst);
122+
.isGreaterThanOrEqualTo(afterFirst).isLessThan(afterFirst + QUERIES);
119123
assertThat(afterFirst).isGreaterThanOrEqualTo(beforeClose);
120124
}
121125

0 commit comments

Comments
 (0)