HhH666-ux commented on PR #40097: URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6443213191
All three P2 findings from the 2026-10-26 review are addressed in 0de97812. **P2-1 — abandoned-state reclamation no longer depends on later SQL or map access** `PENDING_SPANS` is a `ConcurrentHashMap` now, and each `PendingSpan` records the time it was registered. A single daemon scheduler thread (`shardingsphere-opentelemetry-abandoned-span-reclaimer`, started from a static initializer) runs at a fixed delay and ends and releases every entry that is older than a retention bound. Nothing on the SQL execution path and no map access is needed for that to happen, so an abandoned span is released while tracing stays disabled. Switching away from `WeakHashMap` also removes the reason its cleanup never fired: the value kept its own key strongly reachable, so an entry was only expunged when some other operation happened to touch the map. On the retention bound — it is 5 minutes, deliberately far longer than the execution time of an ordinary statement, so that an invocation that is still running is not cut short. The reclaimer ends the span instead of dropping it, so the "span disappears" symptom from #39765 does not come back; the deadline only bounds how long the state is retained. `reclaimAbandonedSpans` is package-private so the test can drive the deadline directly, instead of sleeping until the scheduler thread fires. I did consider a `ReferenceQueue`, but it is still driven by garbage collection, so it cannot make the P2-2 test repeatable. The scheduler thread supplies the "independent of subsequent SQL and map access" half of P2-1, and the deadline is what makes it testable. **P2-2 — repeatable test** `assertAbandonedSpanIsReclaimedWhenCompletionHooksAreSkipped` no longer calls `System.gc()` and no longer sleeps. It registers one span, drives `reclaimAbandonedSpans` with a deadline past the retention bound, and asserts that the span reached the exporter and that the registry is empty. The outcome no longer depends on GC policy, on `-XX:+DisableExplicitGC`, or on scheduling. **P2-3 — coding standards** - `setAccessible` and `Field.get` are gone. The field is read through `Plugins.getMemberAccessor()`, and reflection is limited to `Field` access. - The added line comments are JAVADOC blocks on their test methods now. Neither file contains a `//` comment. - `endSpan` immediately follows `afterMethod`, and `findSpanByStatement` immediately follows `assertNestedFailureDoesNotEndOuterSpan`, its first caller. - `Map.Entry` became an imported `Entry`, which is what the repository's `RegexpSinglelineJava` rule requires. **Verification** This module pins `<compile.java.version>21</compile.java.version>` and only JDK 17 is available here, so I could not run `./mvnw -pl agent/plugins/tracing/type/opentelemetry`. I checked both files with `javac` (the only diagnostics are unresolved third-party packages), and I checked the formatting by hand against `src/resources/spotless/java.xml` (`indent_empty_lines=true`, `lineSplit=200`, `join_wrapped_lines=false`) and against `src/resources/checkstyle.xml` (`FinalParameters`, `DeclarationOrder`, `InnerTypeLast`, `JavadocContentLocation`, `SummaryJavadoc`). CI is the actual verification for this change — if anything goes red I will fix it. -- 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]
