flyingImer commented on code in PR #4061:
URL: https://github.com/apache/polaris/pull/4061#discussion_r3034222678
##########
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -260,37 +256,52 @@ protected void handleTask(
@ActivateRequestContext
protected void handleTaskWithTracing(
- String realmId,
long taskEntityId,
CallContext callContext,
- PolarisPrincipal principal,
+ List<AsyncContextPropagator.RestoreAction> actions,
PolarisEventMetadata eventMetadata,
int attempt) {
- // Note: each call to this method runs in a new CDI request context
-
- realmContextHolder.set(() -> realmId);
- // since this is now a different context we store clone of the principal
in a holder object
- // which essentially reauthenticates the principal. PolarisPrincipal bean
always looks for a
- // principal set in PolarisPrincipalHolder first and assumes that identity
if set.
- polarisPrincipalHolder.set(principal);
+ // Note: each call to this method runs in a new CDI request context.
+ // Restore all propagated context into the fresh request scope.
+ // A restore failure is fatal: running a task without proper realm or
principal context
+ // risks wrong-tenant or wrong-identity execution, so we abort rather than
continue.
+ // The restore loop is inside the try block so the finally block cleans up
any
+ // actions that were already restored before the failure.
+ int restored = 0;
+ try {
+ for (AsyncContextPropagator.RestoreAction action : actions) {
+ action.restore();
+ restored++;
+ }
- if (tracer == null) {
- handleTask(taskEntityId, callContext, eventMetadata, attempt);
- } else {
- Span span =
- tracer
- .spanBuilder("polaris.task")
- .setParent(Context.current())
- .setAttribute(
- TracingFilter.REALM_ID_ATTRIBUTE,
- callContext.getRealmContext().getRealmIdentifier())
- .setAttribute("polaris.task.entity.id", taskEntityId)
- .setAttribute("polaris.task.attempt", attempt)
- .startSpan();
- try (Scope ignored = span.makeCurrent()) {
+ if (tracer == null) {
handleTask(taskEntityId, callContext, eventMetadata, attempt);
- } finally {
- span.end();
+ } else {
+ Span span =
+ tracer
+ .spanBuilder("polaris.task")
+ .setParent(Context.current())
+ .setAttribute(
+ TracingFilter.REALM_ID_ATTRIBUTE,
+ callContext.getRealmContext().getRealmIdentifier())
+ .setAttribute("polaris.task.entity.id", taskEntityId)
+ .setAttribute("polaris.task.attempt", attempt)
+ .startSpan();
+ try (Scope ignored = span.makeCurrent()) {
+ handleTask(taskEntityId, callContext, eventMetadata, attempt);
+ } finally {
+ span.end();
+ }
+ }
+ } finally {
+ // Close in reverse order (LIFO) so that later actions clean up before
+ // earlier ones, matching standard stacked-context conventions.
+ for (int i = restored - 1; i >= 0; i--) {
+ try {
+ actions.get(i).close();
Review Comment:
hmmm, on second thought, makes sense. I now agree this PR doesn't have a
strong enough case for a new SPI. The main value is the propagation itself, not
the extension point.
I'll rework this to a concrete helper along the lines of your
ContextualTaskRunner suggestion, wiring in the three holders directly. MDC will
be a separate follow-up.
--
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]
