HhH666-ux commented on PR #40097: URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6538892666
Pushed `fe2402b`, which takes the opposite trade-off from the previous revisions. Instead of guessing which pending entries are abandoned, this revision stops guessing and narrows the PR to the part that is provably correct. **What changed.** The reclaimer, the enablement generation and its scheduler are gone. What remains is invocation matching: a span is registered under its own `JDBCExecutionUnit`, `onThrowing` only records the throwable, and `afterMethod` is the single exit that ends it. `endAllPendingSpans` still releases everything when the plugin stops. Net change: -81/+6. **On the three findings.** 1. *Premature completion of active queries.* The failure mode is removed rather than mitigated: no code path in this advice ends a span before its own `afterMethod` runs, so a running query can no longer have its span truncated or its failure dropped. 2. *State retained across disable/re-enable.* This is the finding I could not solve from inside the advice, and I no longer try to. `InstanceMethodAdviceExecutor` checks `isPluginEnabled` independently in `adviceBefore`, `adviceThrow` and `adviceAfter`, and reports nothing when it skips the completion hooks. Any in-advice heuristic has to reconstruct that decision from plugin state sampled later — which is exactly what produced the wrong calls in rounds 2 through 6, including the last one. A retained entry costs one span and its execution unit, only in the window where the plugin is disabled between two hooks of one invocation, and it is never exported with a wrong status. Ending a live span is a correctness bug; retaining one entry is a bounded cost that disappears at `close()`. I would rather ship the correct half now and track the rest than keep trading one race for another. 3. *Test calling a non-public helper.* The test went away with the helper. Every remaining case goes through `beforeMethod` / `onThrowing` / `afterMethod` or the plugin lifecycle. **Verification.** `Check - CheckStyle`, `Check - Spotless` and `Check - License` are green, and `CI` — which runs `OpenTelemetryJDBCExecutorCallbackAdviceTest` — passed on this push. **Follow-up.** The clean fix for reclamation is an explicit skip signal from `InstanceMethodAdviceExecutor`, or a disable event on `PluginContext`. That lets the advice release exactly the invocations whose hooks were skipped, with no guessing. It is an `agent/core` change that affects every plugin, so it does not belong in this PR; I will open it separately against #39765. If you would rather keep reclamation in this PR, I will pick it up again once the executor signal exists — but I do not believe it can be made correct from inside the advice alone. -- 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]
