dimas-b commented on code in PR #4061:
URL: https://github.com/apache/polaris/pull/4061#discussion_r3030797844
##########
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:
I think I understand what you'd like to build, but I believe the term
"context propagation SPI" (from PR title) would be really confusing in that
case.
If the idea is to establish a convenience framework for running code inside
some context and make sure the current thread removes traces of that context on
completion, I believe a better approach would to follow the pattern of
`Bootstrapper` (suggested new name: `ContrextualTaskRunner`).
https://github.com/apache/polaris/blob/6c1a12bfd25a77b34e9e803295cc92492dda47b6/runtime/service/src/main/java/org/apache/polaris/service/config/Bootstrapper.java#L39
Inject `RequestIdHolder`, `PolarisPrincipalHolder` and `RealmContextHolder`
into it.
Use them to propagate context internally. No need for a new SPI.
`TaskExecutorImpl` would call:
1. (on task creation) `ContextualAction action =
ContrextualTaskRunner.capture()`
2. (on execution) `action.runWithNewContext( { ... } );`
MDC can be handled internally and cleared upon exit from `runWithContext`
--
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]
