Use isolated request contexts for task execution #1817

dimas-b · 2025-06-05T04:10:21Z

Avoid propagating request contexts into threads running background tasks.
Use fresh request context for each task instead.
Manually propagate realm context into a task from the request that submits the task.
Update DefaultConfigurationStore to cross-check realm IDs in request context and explicit method parameters.
Fix tests to set thread-local data to match CDI context data.

Next step: Simplify DefaultConfigurationStore API to remove contextual arguments and use CDI context instead.

adnanhemani

Hi @dimas-b - I'm also attempting to solve a related problem (#1765), but based on my understanding of Quarkus, I'm not understanding how this solves the problem we are seeing.

From what I got, you are adding the RealmContext object to a new PolarisRequestContext RequestScoped object. Does this inner RealmContext object not still expire at the end of the original request? Or is this working because you've added @ActivateRequestContext to the handleTask method in QuarkusTaskExecutorImpl - and refreshing the polarisRequestContext there? If so, is there any side-effects of doing so?

Additionally, does this fix all existing Tasks or do we need to make additional changes to ensure that all existing tasks start reading from polarisRequestContext?

Also, similar comment as @adutra on #1765 - is there any test that shows what's failing today and that this change fixes that?

dimas-b · 2025-06-05T13:30:10Z

@ActivateRequestContext in combination with .propagated(ThreadContext.NONE) (see it in diff) result in the creation of a new, independent request context for each (async) task.

PolarisRequestContext and all other @RequestScoped objects still expire at the end the request, but this "request" for task is the lifecycle of the QuarkusTaskExecutorImpl.handleTask() method call now.

PolarisRequestContext does not need to be accessed directly by tasks. In most cases relevant objects are injected by CDI.

dimas-b · 2025-06-05T13:31:46Z

is there any test that shows what's failing today and that this change fixes that?

Things are not broken today @gh-yzou fixed hard errors. The problem this PR addresses is the confusion between explicit method parameters and implicit context parameters.

adutra · 2025-06-05T15:51:16Z

...s/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java

+   * #setRealmContext(RealmContext)}.
+   */
+  public RealmContext realmContext() {
+    return realmCtx.get();


Should we add a runtime check here to prevent returning null?

Quarkus already makes a runtime exception if that is the case, but I'll add a check for a more friendly message.

checkState added

adutra · 2025-06-05T15:53:50Z

...kus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java

    callContext = polarisContext;
    CallContext.setCurrentContext(callContext);

+    metaStoreManager = managerFactory.getOrCreateMetaStoreManager(realmContext);


What is the motivation for this change?

the real change is moving CallContext.setCurrentContext(callContext) above, but GH formats it strangely 😅

CallContext.setCurrentContext must happen earlier to make sure "thread local" stuff is aligned with CDI context.

well, after rebasing these changes are not meaningful, indeed 🤷

adutra · 2025-06-05T15:55:48Z

service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java

@@ -51,7 +54,10 @@ public DefaultConfigurationStore(

  @Override
  public <T> @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) {
-    String realm = realmContext.getRealmIdentifier();
+    String realm = realmContextInstance.get().getRealmIdentifier();


Can you explain the rationale here? I thought we would be removing realmContextInstance completely. It's imho more elegant to receive the realm context as a method parameter than injecting an Instance<RealmContext> – especially since this bean is application-scoped, so receiving a realm context as a method parameter aligns more with the common idiom that we have everywhere in Polaris where an application-scoped bean exposes a method that takes a realm context and returns something for that realm (e.g. all the getOrCreateXYZ methods).

yes, that will not be needed anymore.

My thinking goes in the other direction. I'd like to make DeafaultConfigurationStore a proper CDI bean getting realm ID from the request context. Ideally Instance<RealmContext> simply becomes RealmContext.

Eventually, I'd like to remove the RealmContext parameter.

I believe refactoring the code and internal interfaces to be free from RealmContext parameters in methods would be beneficial. Most of the code is written to operate within a realm. Managing various objects (e.g. meta store instances) for multiple realms feels like an infrastructure concern. WDYT?

adutra · 2025-06-05T15:56:06Z

...service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogViewTest.java

@@ -175,15 +175,14 @@ public void before(TestInfo testInfo) {
            diagServices,
            configurationStore,
            Clock.systemDefaultZone());
+    CallContext.setCurrentContext(polarisContext);


Same: is this change required?

Just moving CallContext.setCurrentContext above the calls that rely on it -- to ensure CDI context has the same data and thread locals.

well, after rebasing these changes are not meaningful, indeed 🤷

...s/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java

gh-yzou · 2025-06-05T17:18:32Z

...s/service/src/main/java/org/apache/polaris/service/quarkus/task/QuarkusTaskExecutorImpl.java

@@ -49,9 +52,11 @@ public QuarkusTaskExecutorImpl(
      MetaStoreManagerFactory metaStoreManagerFactory,
      TaskFileIOSupplier fileIOSupplier,
      Tracer tracer,
-      PolarisEventListener polarisEventListener) {
+      PolarisEventListener polarisEventListener,


I don't fully understand how the propagation works, but will we be able to add some test to make sure the context is propagated correctly?

This is covered by existing tests that produce tasks.

It works like this:

Quarkus create a new request context when handleTask is called

The code below this like puts the RealmContext for the tasks into polarisRequestContext

The Quarkus proxy for polarisRequestContext redirects this "set" call to the instance in the current request context

When something needs a RealmContext during the execution of a task, Quarkus invokes QuarkusProducers.realmContext(PolarisRequestContext context) and gives it the same PolarisRequestContext instance that was used in step 3.

For HTTP requests step 1 is the start of the request at the REST API layer, in which case the RealmContext is derived from HTTP headers by a filter.

This whole workflow is very similar to how it worked before, but we use a custom "holder" object instead of ContainerRequestContext, which allows us to support non-HTTP requests (i.e. async tasks).

well, after rebasing test coverage is gone 🤷

This is covered by existing tests that produce tasks.

Unfortunately I don't think we have such case running in our CI today, otherwise, it would have caught the problem when we do ConfigurationStore injection. The only case i know which triggers background task is purge, which was caught by an aws regression test, which can only run manually today.

I am trying to add a spark integration test here https://github.com/apache/polaris/pull/1825/files, but runs into following error when do drop with purge

2025-06-05 18:36:21,503 INFO [org.apa.pol.ser.exc.IcebergExceptionMapper] [,POLARIS] [,,,] (executor-thread-1) Handling runtimeException Principal 'root' with activated PrincipalRoles '[]' and activated grants via '[service_admin, catalog_admin]' is not authorized for op DROP_TABLE_WITH_PURGE

i am wondering if you know how the catalogAPI privilege is setup with quarkus test?

It looks like it's using default privileges in this case. Related slack discussion: https://apache-polaris.slack.com/archives/C084XDM50CB/p1748381388095509

Re: test coverage : when my new code had checkState assertions, it caused CI failures in the tests I had to modify. Unfortunately, after rebasing request context stuff is no longer injected into the default config impl., so it cannot perform those checks 🤷 I'll address this if the community decides to go ahead with this PR.

It looks like it's using default privileges in this case. Related slack discussion: https://apache-polaris.slack.com/archives/C084XDM50CB/p1748381388095509

Thanks! let me see if I can get this test work.

gh-yzou · 2025-06-05T17:19:42Z

...s/service/src/main/java/org/apache/polaris/service/quarkus/task/QuarkusTaskExecutorImpl.java

@@ -73,6 +79,7 @@ protected void handleTask(long taskEntityId, CallContext callContext, int attemp
            .setAttribute("polaris.task.attempt", attempt)
            .startSpan();
    try (Scope ignored = span.makeCurrent()) {
+      polarisRequestContext.setRealmContext(callContext.getRealmContext());


with the propagation now, will we be able to remove the code here https://github.com/dimas-b/polaris/blob/main/polaris-service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java#L71

yes, good point. I'll do it later if people agree on the general approach in this PR.

Actually, i think we probably need to propagate the whole callcontext, instead of just the realmContext, because the whole callContext is needed for the background executor, the realmContext is just what needed for getConfiguration, which is called during the task execution.

Eventually - yes. With the recent config changes this PR is no longer meaningful in isolation, but it shows how CDI can be done for tasks.

I also opened a bigger related proposal on the dev ML: https://lists.apache.org/thread/0cyrzft2oon28otxlmmhvd5671rd3r3d

* Avoid propagating request contexts into threads running background tasks. * Use fresh request context for each task instead. * Manually propagate realm context into a task from the request that submits the task. * Fix tests to set thread-local data to match CDI context data. Next step: Simplify DefaultConfigurationStore API to remove contextual arguments and use CDI context instead.

dimas-b · 2025-06-05T22:47:01Z

So, after rebasing the new way of handling request context in tasks is indeed not very meaningful, because tasks no longer use request-scoped beans.

However, I'd like this PR to act as an illustration for the proposal of moving all RealmContext from explicit method parameters to object fields to be injected by CDI (or manually). This PR shows that it is possible. I'll open a dev ML thread on this.

github-project-automation bot added this to Basic Kanban Board Jun 5, 2025

dimas-b requested review from adutra and ashvina as code owners June 5, 2025 04:10

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jun 5, 2025

dimas-b requested review from dennishuo, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain, ebyhr, ajantha-bhat, HonahX, singhpk234 and pingtimeout as code owners June 5, 2025 04:10

adnanhemani reviewed Jun 5, 2025

View reviewed changes

adutra reviewed Jun 5, 2025

View reviewed changes

gh-yzou reviewed Jun 5, 2025

View reviewed changes

dimas-b force-pushed the task-request-ctx branch from f5b4f14 to 35f0871 Compare June 5, 2025 22:42

dimas-b force-pushed the task-request-ctx branch from 35f0871 to cb51efb Compare June 5, 2025 22:42

Use isolated request contexts for task execution #1817

Are you sure you want to change the base?

Use isolated request contexts for task execution #1817

Uh oh!

Conversation

dimas-b commented Jun 5, 2025

Uh oh!

adnanhemani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Jun 5, 2025

Uh oh!

dimas-b commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Jun 5, 2025

Uh oh!

Uh oh!

adnanhemani left a comment •

edited

Loading

dimas-b Jun 5, 2025 •

edited

Loading