HhH666-ux commented on PR #40097: URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6395832085
Thanks for the thorough second round, @terrymanu. All three blocking issues are addressed in `a77c3d1`, which replaces the thread-local deque with a registry keyed by the execution unit of the invocation that registered the span. **Blocking #1 — invocation matching.** `recordExecuteInfo` registers the started span under its own `JDBCExecutionUnit`, and the completion hooks look the state up by `args[0]` (`afterMethod` removes, `onThrowing` only records). A nested callback whose `beforeMethod` failed before registering a span therefore leaves nothing for its completion hooks to consume — they can no longer end the outer span or mark it failed. The requested regression (`assertNestedFailedBeforeDoesNotEndOuterSpan`) drives the complete `InstanceMethodAdviceExecutor` lifecycle: the inner invocation targets a callback without a `resourceMetaData` field, so its `adviceBefore` fails and is swallowed, the executor still runs the inner completion hooks inside the outer invocation's `callable`, and the outer callable then throws. The result matches your merge-base differential: exactly one span exported, `ERROR`, one recorded exception event. **Blocking #2 — reclamation on skipped completion.** There is no lifecycle signal for a skipped hook, so the registry keys are weak references: when the completion hooks are skipped (plugin disabled between setup and completion), the entry stays only as long as the execution unit is reachable, and the next garbage collection reclaims the span and its SQL attributes — retention is bounded by the invocation, not the worker thread. As you noted, invocation matching alone does not resolve this, which is why the weak key is load-bearing rather than incidental. `assertPendingSpanIsReclaimedWhenCompletionHooksAreSkipped` registers a span without any completion hooks, drops the last strong reference to the unit, and asserts the registry drains to zero after a GC. **Blocking #3 — written coding standards.** `PendingSpan` now uses `@RequiredArgsConstructor` + `@Getter` with a narrow `@Setter` on `throwable` only; the two test constructor statements are joined to single lines within the line limit; the observation locals in the nested and concurrent tests are renamed `actualSpanItems`. One pom note: the executor-lifecycle regression needs `shardingsphere-agent-core` as a test-scoped dependency of the module — it brings only `byte-buddy` and `snakeyaml` transitively. Local verification, with `M=agent/plugins/tracing/type/opentelemetry`: focused test run — 7/7 passed (`assertMethod`, `assertMethodWithoutParentSpan`, `assertExceptionHandle`, `assertNestedFailureDoesNotEndOuterSpan`, `assertNestedFailedBeforeDoesNotEndOuterSpan`, `assertPendingSpanIsReclaimedWhenCompletionHooksAreSkipped`, `assertConcurrentInvocations`); `spotless:check checkstyle:check -Pcheck` — exit 0, zero violations, both governed files clean. -- 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]
