Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2138485909
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java:
##
@@ -297,7 +297,7 @@ public TokenBroker tokenBroker(
public ManagedExecutor taskExecutor(TaskHandlerConfiguration config) {
return SmallRyeManagedExecutor.builder()
.injectionPointName("task-executor")
-.propagated(ThreadContext.ALL_REMAINING)
+.propagated(ThreadContext.NONE)
Review Comment:
Good point! Will do a bit later, when I get to work on this again :)
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
adutra commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2138197158
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java:
##
@@ -297,7 +297,7 @@ public TokenBroker tokenBroker(
public ManagedExecutor taskExecutor(TaskHandlerConfiguration config) {
return SmallRyeManagedExecutor.builder()
.injectionPointName("task-executor")
-.propagated(ThreadContext.ALL_REMAINING)
+.propagated(ThreadContext.NONE)
Review Comment:
This might be a bit too much? How about:
```java
return SmallRyeManagedExecutor.builder()
.injectionPointName("task-executor")
.propagated(ThreadContext.ALL_REMAINING)
.cleared(ThreadContext.CDI)
.maxAsync(config.maxConcurrentTasks())
.maxQueued(config.maxQueuedTasks())
.build();
```
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2132573217
##
quarkus/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());
Review Comment:
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
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
gh-yzou commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2132545559
##
quarkus/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,
Review Comment:
```
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.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
gh-yzou commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2132542508
##
quarkus/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());
Review Comment:
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.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2132162610
##
quarkus/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,
Review Comment:
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 :shrug: I'll address this if the community decides to go
ahead with this PR.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2132159483
##
quarkus/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,
Review Comment:
It looks like it's using default privileges in this case. Related slack
discussion:
https://apache-polaris.slack.com/archives/C084XDM50CB/p1748381388095509
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
gh-yzou commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2131457168
##
quarkus/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,
Review Comment:
```
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?
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on PR #1817: URL: https://github.com/apache/polaris/pull/1817#issuecomment-2946695142 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. -- 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: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130754696
##
quarkus/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,
Review Comment:
well, after rebasing test coverage is gone :shrug:
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130751164
##
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java:
##
@@ -242,11 +238,16 @@ public void before(TestInfo testInfo) {
diagServices,
configurationStore,
clock);
-this.entityManager =
realmEntityManagerFactory.getOrCreateEntityManager(realmContext);
-
callContext = polarisContext;
CallContext.setCurrentContext(callContext);
+metaStoreManager =
managerFactory.getOrCreateMetaStoreManager(realmContext);
Review Comment:
well, after rebasing these changes are not meaningful, indeed :shrug:
##
quarkus/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);
Review Comment:
well, after rebasing these changes are not meaningful, indeed :shrug:
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130748972
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * A container for request-scoped information discovered during request
execution.
+ *
+ * This is an equivalent for {@link ContainerRequestContext}, but for use
in non-HTTP requests.
+ */
+@RequestScoped
+public class PolarisRequestContext {
+ private final AtomicReference realmCtx = new
AtomicReference<>();
+
+ /**
+ * Records the {@link RealmContext} that applies to current request. The
realm context may be
+ * determined from REST API header or by passing explicit realm ID values
from one CDI context to
+ * another.
+ *
+ * During the execution of a particular request, this method should be
called before {@link
+ * #realmContext()}.
+ */
+ public void setRealmContext(RealmContext rc) {
+System.out.println("SET: " + this + ": RC: " + rc);
+realmCtx.set(rc);
+ }
+
+ /**
+ * Returns the realm context for this request previously set via {@link
+ * #setRealmContext(RealmContext)}.
+ */
+ public RealmContext realmContext() {
+return realmCtx.get();
Review Comment:
checkState added
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130619682
##
quarkus/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());
Review Comment:
yes, good point.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130614922
##
quarkus/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,
Review Comment:
This is covered by existing tests that produce tasks.
It works like this:
1. Quarkus create a new request context when `handleTask` is called
2. The code below this like puts the `RealmContext` for the tasks into
`polarisRequestContext`
3. The Quarkus proxy for `polarisRequestContext` redirects this "set" call
to the instance in the current request context
4. 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).
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130619682
##
quarkus/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());
Review Comment:
yes, good point. I'll do it later if people agree on the general approach in
this PR.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130583920
##
service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java:
##
@@ -51,7 +54,10 @@ public DefaultConfigurationStore(
@Override
public @Nullable T getConfiguration(@Nonnull RealmContext realmContext,
String configName) {
-String realm = realmContext.getRealmIdentifier();
+String realm = realmContextInstance.get().getRealmIdentifier();
Review Comment:
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` 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?
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130586187
##
quarkus/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);
Review Comment:
Just moving `CallContext.setCurrentContext` above the calls that rely on it
-- to ensure CDI context has the same data and thread locals.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130587984
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * A container for request-scoped information discovered during request
execution.
+ *
+ * This is an equivalent for {@link ContainerRequestContext}, but for use
in non-HTTP requests.
+ */
+@RequestScoped
+public class PolarisRequestContext {
+ private final AtomicReference realmCtx = new
AtomicReference<>();
+
+ /**
+ * Records the {@link RealmContext} that applies to current request. The
realm context may be
+ * determined from REST API header or by passing explicit realm ID values
from one CDI context to
+ * another.
+ *
+ * During the execution of a particular request, this method should be
called before {@link
+ * #realmContext()}.
+ */
+ public void setRealmContext(RealmContext rc) {
+System.out.println("SET: " + this + ": RC: " + rc);
Review Comment:
sorry, debugging leftover - removed.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130558285
##
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java:
##
@@ -242,11 +238,16 @@ public void before(TestInfo testInfo) {
diagServices,
configurationStore,
clock);
-this.entityManager =
realmEntityManagerFactory.getOrCreateEntityManager(realmContext);
-
callContext = polarisContext;
CallContext.setCurrentContext(callContext);
+metaStoreManager =
managerFactory.getOrCreateMetaStoreManager(realmContext);
Review Comment:
the real change is moving `CallContext.setCurrentContext(callContext)`
above, but GH formats it strangely :sweat_smile:
`CallContext.setCurrentContext` must happen earlier to make sure "thread
local" stuff is aligned with CDI context.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2130553174
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * A container for request-scoped information discovered during request
execution.
+ *
+ * This is an equivalent for {@link ContainerRequestContext}, but for use
in non-HTTP requests.
+ */
+@RequestScoped
+public class PolarisRequestContext {
+ private final AtomicReference realmCtx = new
AtomicReference<>();
+
+ /**
+ * Records the {@link RealmContext} that applies to current request. The
realm context may be
+ * determined from REST API header or by passing explicit realm ID values
from one CDI context to
+ * another.
+ *
+ * During the execution of a particular request, this method should be
called before {@link
+ * #realmContext()}.
+ */
+ public void setRealmContext(RealmContext rc) {
+System.out.println("SET: " + this + ": RC: " + rc);
+realmCtx.set(rc);
+ }
+
+ /**
+ * Returns the realm context for this request previously set via {@link
+ * #setRealmContext(RealmContext)}.
+ */
+ public RealmContext realmContext() {
+return realmCtx.get();
Review Comment:
Quarkus already makes a runtime exception if that is the case, but I'll add
a check for a more friendly message.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
gh-yzou commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2129382758
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * A container for request-scoped information discovered during request
execution.
+ *
+ * This is an equivalent for {@link ContainerRequestContext}, but for use
in non-HTTP requests.
+ */
+@RequestScoped
+public class PolarisRequestContext {
+ private final AtomicReference realmCtx = new
AtomicReference<>();
+
+ /**
+ * Records the {@link RealmContext} that applies to current request. The
realm context may be
+ * determined from REST API header or by passing explicit realm ID values
from one CDI context to
+ * another.
+ *
+ * During the execution of a particular request, this method should be
called before {@link
+ * #realmContext()}.
+ */
+ public void setRealmContext(RealmContext rc) {
+System.out.println("SET: " + this + ": RC: " + rc);
Review Comment:
instead of print can we use LOGGER
##
quarkus/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,
Review Comment:
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?
##
quarkus/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());
Review Comment:
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
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
gh-yzou commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2129381164
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * A container for request-scoped information discovered during request
execution.
+ *
+ * This is an equivalent for {@link ContainerRequestContext}, but for use
in non-HTTP requests.
+ */
+@RequestScoped
+public class PolarisRequestContext {
+ private final AtomicReference realmCtx = new
AtomicReference<>();
+
+ /**
+ * Records the {@link RealmContext} that applies to current request. The
realm context may be
+ * determined from REST API header or by passing explicit realm ID values
from one CDI context to
+ * another.
+ *
+ * During the execution of a particular request, this method should be
called before {@link
+ * #realmContext()}.
+ */
+ public void setRealmContext(RealmContext rc) {
+System.out.println("SET: " + this + ": RC: " + rc);
+realmCtx.set(rc);
+ }
+
+ /**
+ * Returns the realm context for this request previously set via {@link
+ * #setRealmContext(RealmContext)}.
+ */
+ public RealmContext realmContext() {
+return realmCtx.get();
Review Comment:
+1,
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
gh-yzou commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2129376163
##
service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java:
##
@@ -51,7 +54,10 @@ public DefaultConfigurationStore(
@Override
public @Nullable T getConfiguration(@Nonnull RealmContext realmContext,
String configName) {
-String realm = realmContext.getRealmIdentifier();
+String realm = realmContextInstance.get().getRealmIdentifier();
Review Comment:
yes, that will not be needed anymore.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
adutra commented on code in PR #1817:
URL: https://github.com/apache/polaris/pull/1817#discussion_r2129171982
##
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/PolarisRequestContext.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.quarkus.config;
+
+import jakarta.enterprise.context.RequestScoped;
+import jakarta.ws.rs.container.ContainerRequestContext;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.context.RealmContext;
+
+/**
+ * A container for request-scoped information discovered during request
execution.
+ *
+ * This is an equivalent for {@link ContainerRequestContext}, but for use
in non-HTTP requests.
+ */
+@RequestScoped
+public class PolarisRequestContext {
+ private final AtomicReference realmCtx = new
AtomicReference<>();
+
+ /**
+ * Records the {@link RealmContext} that applies to current request. The
realm context may be
+ * determined from REST API header or by passing explicit realm ID values
from one CDI context to
+ * another.
+ *
+ * During the execution of a particular request, this method should be
called before {@link
+ * #realmContext()}.
+ */
+ public void setRealmContext(RealmContext rc) {
+System.out.println("SET: " + this + ": RC: " + rc);
+realmCtx.set(rc);
+ }
+
+ /**
+ * Returns the realm context for this request previously set via {@link
+ * #setRealmContext(RealmContext)}.
+ */
+ public RealmContext realmContext() {
+return realmCtx.get();
Review Comment:
Should we add a runtime check here to prevent returning null?
##
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/PolarisAuthzTestBase.java:
##
@@ -242,11 +238,16 @@ public void before(TestInfo testInfo) {
diagServices,
configurationStore,
clock);
-this.entityManager =
realmEntityManagerFactory.getOrCreateEntityManager(realmContext);
-
callContext = polarisContext;
CallContext.setCurrentContext(callContext);
+metaStoreManager =
managerFactory.getOrCreateMetaStoreManager(realmContext);
Review Comment:
What is the motivation for this change?
##
quarkus/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);
Review Comment:
Same: is this change required?
##
service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java:
##
@@ -51,7 +54,10 @@ public DefaultConfigurationStore(
@Override
public @Nullable T getConfiguration(@Nonnull RealmContext realmContext,
String configName) {
-String realm = realmContext.getRealmIdentifier();
+String realm = realmContextInstance.get().getRealmIdentifier();
Review Comment:
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` –
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).
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on PR #1817: URL: https://github.com/apache/polaris/pull/1817#issuecomment-2944345138 `@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. -- 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: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Use isolated request contexts for task execution [polaris]
dimas-b commented on PR #1817: URL: https://github.com/apache/polaris/pull/1817#issuecomment-2944357384 > 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. -- 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: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
