HhH666-ux commented on PR #40097: URL: https://github.com/apache/shardingsphere/pull/40097#issuecomment-6232679908
Thanks for the review, @terrymanu. **Blocking #1 — exception completion** is addressed in `0107509c`. All span completion now goes through a single exit: - `recordExecuteInfo` pushes a `PendingSpan` (span + throwable) onto the thread-local deque. - `onThrowing` no longer ends anything — it only records the throwable on the pending span it belongs to. - `afterMethod` is the only place that ends a span, and it applies `StatusCode.ERROR` + `recordException` when a throwable was recorded. So the inner invocation's error hook can no longer close a span followed by a final hook that reports `OK` for the still-running outer one. For your reproducer (inner fails, exception propagates to the outer call): inner `onThrowing` marks the inner span, inner `afterMethod` ends it as `ERROR`, the outer `onThrowing` marks the outer span, and the outer `afterMethod` ends it as `ERROR` with the exception recorded. `assertNestedFailureDoesNotEndOuterSpan` covers exactly this lifecycle, including the case where the outer call completes normally after the inner failure (`assertConcurrentInvocations` covers the overlapping case). **Blocking #2 — style** is fixed. `nullValue` is replaced with `assertNull`, the imports are reordered, and the asserted locals now carry the `actual` prefix. On the current head all three gates are green: `Check - CheckStyle`, `Check - Spotless`, `Check - License`. **About the red CI**: it was not caused by this change. The `CI` job failed to compile `FirebirdGetBlobSegmentCommandExecutorTest` in `proxy/frontend/dialect/firebird` — `registerBlob` gained a fourth `boolean` parameter on master after this branch was created. That is fixed on master now, so I pushed an empty commit to re-run the suite against the current base. One open design question for you: completion still matches by LIFO order on the thread-local deque, which follows the executor's nested call semantics. If you would rather have explicit per-invocation identity, I can switch the lifecycle to `Context`/`Scope` so each hook closes exactly the span it opened. Happy to do it either way. -- 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]
