rishabhdaim commented on PR #2895:
URL: https://github.com/apache/jackrabbit-oak/pull/2895#issuecomment-5379376932
๐ค **AI-generated review** โ automated code review via GitHub Copilot CLI
(change-reviewer). 10 lenses were dispatched (correctness, tests, consistency,
compatibility, design, concurrency, spec-compliance, history, reversibility,
performance); findings below are the subset that survived independent
confidence verification against the current diff. Treat as input for human
review, not a final verdict.
Findings outside this diff:
`oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/CacheStatsSnapshot.java`
(~line 124) โ ๐ **P2** ยท ๐งช Tests
**๐ Issue:** The new `plus()` method uses `Math.addExact` (throws
`ArithmeticException` on overflow) replacing Guava's silently-wrapping `+`, but
has zero test coverage even though it's used by
`WriterCacheManager.accumulateRecordCacheStats` to accumulate long-lived JMX
cache stats.
**๐ ๏ธ Fix:** Add a `CacheStatsSnapshotTest` case for `plus()`, including an
overflow boundary case asserting the new `ArithmeticException`.
`oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/CacheStats.java`
(~line 1) โ ๐ **P2** ยท ๐ Consistency
**๐ Issue:** `PLAN.md`/`TASKS.md` document deprecating `CacheStats`
(Guava-constructor) and keeping `CacheStatsAdapter` as canonical, but this PR
does the opposite โ it deletes `CacheStatsAdapter` and keeps `CacheStats` as
the surviving, non-deprecated class with no `@Deprecated` annotation added.
**๐ ๏ธ Fix:** Update `PLAN.md`/`TASKS.md` to reflect the chosen direction, or
follow the documented plan instead.
`oak-core-spi/src/test/java/org/apache/jackrabbit/oak/cache/impl/CacheBuilderTest.java`
(~line 236) โ ๐ก **P3** ยท ๐ Consistency
**๐ Issue:** The javadoc ("`CacheStatsAdapter` exposes stats...") and test
method name `oakCacheStatsAdapterBridgesOakStats()` still reference the deleted
`CacheStatsAdapter` class, even though the method body now constructs
`CacheStats`.
**๐ ๏ธ Fix:** Update the javadoc and rename the test method to reference
`CacheStats`.
`oak-lucene/pom.xml` (~line 132) โ ๐ด **P1** ยท ๐ Compatibility
**๐ Issue:** `Embed-Dependency` still inlines the deleted
`CacheStatsAdapter.class` from `oak-core-spi`, and the renamed replacement
`cache/api/CacheStats.class` is missing from the list entirely (the existing
`CacheStats.class` entry is a different, unrelated package).
**๐ฅ Impact:** The bnd inline path list references a class that no longer
exists in the embedded jar, risking a build/packaging failure or a silently
incomplete bundle that reintroduces the classloader-split `VerifyError` this
entry was added to prevent.
**๐ ๏ธ Fix:** Remove
`org/apache/jackrabbit/oak/cache/api/CacheStatsAdapter.class` and add
`org/apache/jackrabbit/oak/cache/api/CacheStats.class` to the inline list, then
re-verify `IndexVersionSelectionIT`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
