You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "eric-maynard (via GitHub)" <gi...@apache.org> on 2025/02/13 21:46:42 UTC
[PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
eric-maynard opened a new pull request, #1000:
URL: https://github.com/apache/polaris/pull/1000
This fully reverts #589, returning CallContext usage to its previous state before 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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1966408700
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -28,57 +27,47 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.polaris.core.PolarisConfigurationStore;
-import org.apache.polaris.core.PolarisDiagnostics;
-import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.TaskEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
-import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Given a list of registered {@link TaskHandler}s, execute tasks asynchronously with the provided
- * {@link RealmContext}.
+ * {@link CallContext}.
*/
public class TaskExecutorImpl implements TaskExecutor {
private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutorImpl.class);
private static final long TASK_RETRY_DELAY = 1000;
+ private final CallContext callContext;
private final Executor executor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
- private final PolarisConfigurationStore configurationStore;
- private final PolarisDiagnostics diagnostics;
private final TaskFileIOSupplier fileIOSupplier;
- private final Clock clock;
private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
public TaskExecutorImpl(
+ CallContext callContext,
Review Comment:
Yeah looks like with the latest fix plumbing `ctx` through `handleTask` it should work now. In general it's probably best to continue handling CallContext somewhat explicitly because even though right now the "background processing" is able to just be best-effort within the same process, the design of having TaskEntity persistence really is supposed to reflect reliable task execution on server failures.
In which case, we need a formal model for creating a "SystemCallContext" of sorts that may or may not "impersonate" parts of the original call context that resulted in the system event. And we'd need to be able to serialize such CallContext state into the TaskEntity and restore it on server startup to create the CallContext with which to perform the system tasks.
Incidentally, this also means there are intended flows that aren't technically `@RequestScoped`.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965978795
##########
polaris-core/src/main/java/org/apache/polaris/core/context/CallContext.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.core.context;
+
+import jakarta.annotation.Nonnull;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.iceberg.io.CloseableGroup;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Stores elements associated with an individual REST request such as RealmContext, caller
+ * identity/role, authn/authz, etc. This class is distinct from RealmContext because implementations
+ * may need to first independently resolve a RealmContext before resolving the identity/role
+ * elements of the CallContext that reside exclusively within the resolved Realm. For example, the
+ * principal/role entities may be defined within a Realm-specific persistence layer, and the
+ * underlying nature of the persistence layer may differ between different realms.
+ */
+public interface CallContext extends AutoCloseable {
+ InheritableThreadLocal<CallContext> CURRENT_CONTEXT = new InheritableThreadLocal<>();
Review Comment:
Yeah after doing this revert and debugging all the test issues with ThreadLocal I am very eager to see ThreadLocal gone and injection used consistently.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965750566
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -28,57 +27,47 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.polaris.core.PolarisConfigurationStore;
-import org.apache.polaris.core.PolarisDiagnostics;
-import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.TaskEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
-import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Given a list of registered {@link TaskHandler}s, execute tasks asynchronously with the provided
- * {@link RealmContext}.
+ * {@link CallContext}.
*/
public class TaskExecutorImpl implements TaskExecutor {
private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutorImpl.class);
private static final long TASK_RETRY_DELAY = 1000;
+ private final CallContext callContext;
private final Executor executor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
- private final PolarisConfigurationStore configurationStore;
- private final PolarisDiagnostics diagnostics;
private final TaskFileIOSupplier fileIOSupplier;
- private final Clock clock;
private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
public TaskExecutorImpl(
+ CallContext callContext,
Review Comment:
I'm wondering: is this a clean revert? Why isn't this class back to the exact state it had before the removal of `CallContext`?
According to git log, it should correspond to commit 0a2f9df435cde79f65fed49a20a40bd96f5d1f37, which is the commit before the `CallContext` removal by b84f4624.
So this class should look like this:
https://github.com/apache/polaris/blob/0a2f9df435cde79f65fed49a20a40bd96f5d1f37/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java
In that commit, `CallContext` wasn't being injected through the constructor. And afaict, it wasn't either in any of the previous revisions of this class.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965977740
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -91,39 +80,45 @@ public void addTaskHandler(TaskHandler taskHandler) {
}
/**
- * Register a {@link RealmContext} for a specific task id. That task will be loaded and executed
- * asynchronously with a copy of the provided {@link RealmContext} (because the realm context is a
- * request-scoped component).
+ * Register a {@link CallContext} for a specific task id. That task will be loaded and executed
+ * asynchronously with a clone of the provided {@link CallContext}.
*/
@Override
- public void addTaskHandlerContext(long taskEntityId, RealmContext realmContext) {
- tryHandleTask(taskEntityId, RealmContext.copyOf(realmContext), null, 1);
+ public void addTaskHandlerContext(long taskEntityId, CallContext callContext) {
+ // Unfortunately CallContext is a request-scoped bean and must be cloned now,
+ // because its usage inside the TaskExecutor thread pool will outlive its
+ // lifespan, so the original CallContext will eventually be closed while
+ // the task is still running.
+ // Note: PolarisCallContext has request-scoped beans as well, and must be cloned.
+ // FIXME replace with context propagation?
+ CallContext clone = CallContext.copyOf(callContext);
+ tryHandleTask(taskEntityId, clone, null, 1);
}
private @Nonnull CompletableFuture<Void> tryHandleTask(
- long taskEntityId, RealmContext realmContext, Throwable e, int attempt) {
+ long taskEntityId, CallContext callContext, Throwable e, int attempt) {
if (attempt > 3) {
return CompletableFuture.failedFuture(e);
}
return CompletableFuture.runAsync(
- () -> handleTask(taskEntityId, realmContext, attempt), executor)
+ () -> handleTask(taskEntityId, callContext, attempt), executor)
.exceptionallyComposeAsync(
(t) -> {
LOGGER.warn("Failed to handle task entity id {}", taskEntityId, t);
- return tryHandleTask(taskEntityId, realmContext, t, attempt + 1);
+ return tryHandleTask(taskEntityId, callContext, t, attempt + 1);
},
CompletableFuture.delayedExecutor(
TASK_RETRY_DELAY * (long) attempt, TimeUnit.MILLISECONDS, executor));
}
- protected void handleTask(long taskEntityId, RealmContext realmContext, int attempt) {
+ protected void handleTask(long taskEntityId, CallContext ctx, int attempt) {
+ // set the call context INSIDE the async task
+ CallContext.setCurrentContext(ctx);
Review Comment:
Changed per this comment -- good catch!
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo merged PR #1000:
URL: https://github.com/apache/polaris/pull/1000
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1964886165
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -91,39 +80,45 @@ public void addTaskHandler(TaskHandler taskHandler) {
}
/**
- * Register a {@link RealmContext} for a specific task id. That task will be loaded and executed
- * asynchronously with a copy of the provided {@link RealmContext} (because the realm context is a
- * request-scoped component).
+ * Register a {@link CallContext} for a specific task id. That task will be loaded and executed
+ * asynchronously with a clone of the provided {@link CallContext}.
*/
@Override
- public void addTaskHandlerContext(long taskEntityId, RealmContext realmContext) {
- tryHandleTask(taskEntityId, RealmContext.copyOf(realmContext), null, 1);
+ public void addTaskHandlerContext(long taskEntityId, CallContext callContext) {
+ // Unfortunately CallContext is a request-scoped bean and must be cloned now,
+ // because its usage inside the TaskExecutor thread pool will outlive its
+ // lifespan, so the original CallContext will eventually be closed while
+ // the task is still running.
+ // Note: PolarisCallContext has request-scoped beans as well, and must be cloned.
+ // FIXME replace with context propagation?
+ CallContext clone = CallContext.copyOf(callContext);
+ tryHandleTask(taskEntityId, clone, null, 1);
}
private @Nonnull CompletableFuture<Void> tryHandleTask(
- long taskEntityId, RealmContext realmContext, Throwable e, int attempt) {
+ long taskEntityId, CallContext callContext, Throwable e, int attempt) {
if (attempt > 3) {
return CompletableFuture.failedFuture(e);
}
return CompletableFuture.runAsync(
- () -> handleTask(taskEntityId, realmContext, attempt), executor)
+ () -> handleTask(taskEntityId, callContext, attempt), executor)
.exceptionallyComposeAsync(
(t) -> {
LOGGER.warn("Failed to handle task entity id {}", taskEntityId, t);
- return tryHandleTask(taskEntityId, realmContext, t, attempt + 1);
+ return tryHandleTask(taskEntityId, callContext, t, attempt + 1);
},
CompletableFuture.delayedExecutor(
TASK_RETRY_DELAY * (long) attempt, TimeUnit.MILLISECONDS, executor));
}
- protected void handleTask(long taskEntityId, RealmContext realmContext, int attempt) {
+ protected void handleTask(long taskEntityId, CallContext ctx, int attempt) {
+ // set the call context INSIDE the async task
+ CallContext.setCurrentContext(ctx);
Review Comment:
I suppose we could also do both to "be safe", but in general any `setCurrentContext` that isn't paired up with a `try {...} finally { CallContext.setCurrentContext(null); }` seems dangerous.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965946104
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -28,57 +27,47 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.polaris.core.PolarisConfigurationStore;
-import org.apache.polaris.core.PolarisDiagnostics;
-import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.TaskEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
-import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Given a list of registered {@link TaskHandler}s, execute tasks asynchronously with the provided
- * {@link RealmContext}.
+ * {@link CallContext}.
*/
public class TaskExecutorImpl implements TaskExecutor {
private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutorImpl.class);
private static final long TASK_RETRY_DELAY = 1000;
+ private final CallContext callContext;
private final Executor executor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
- private final PolarisConfigurationStore configurationStore;
- private final PolarisDiagnostics diagnostics;
private final TaskFileIOSupplier fileIOSupplier;
- private final Clock clock;
private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
public TaskExecutorImpl(
+ CallContext callContext,
Review Comment:
No, it's definitely not a clean revert unfortunately. There were very many conflicts with the revert applied. Taking a look at `TaskExecutorImpl.java`
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965144779
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -28,57 +27,47 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.polaris.core.PolarisConfigurationStore;
-import org.apache.polaris.core.PolarisDiagnostics;
-import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.TaskEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
-import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Given a list of registered {@link TaskHandler}s, execute tasks asynchronously with the provided
- * {@link RealmContext}.
+ * {@link CallContext}.
*/
public class TaskExecutorImpl implements TaskExecutor {
private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutorImpl.class);
private static final long TASK_RETRY_DELAY = 1000;
+ private final CallContext callContext;
private final Executor executor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
- private final PolarisConfigurationStore configurationStore;
- private final PolarisDiagnostics diagnostics;
private final TaskFileIOSupplier fileIOSupplier;
- private final Clock clock;
private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
public TaskExecutorImpl(
+ CallContext callContext,
Review Comment:
Yes,`CallContext` should not be injected but cloned. Here is how it was handled before:
https://github.com/apache/polaris/blob/4409252d135f62e9f9ae09d943c0c0f720c03efa/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java#L83-L92
The reason is that `CallContext` being a request-scoped bean, it becomes tricky to pass it on to a different thread pool, as the instance might survive there longer than the HTTP request itself. Thus it becomes necessary to create a copy of it and pass the copy to the executor. (The copy must be a deep clone: `PolarisCallContext` _must_ be cloned too, especially because of the metastore session bean.)
Side note 1: Quarkus has context propagation techniques, and in fact, the `CallContext` bean would be properly propagated to the executor, because the executor is a Quarkus-managed executor:
https://github.com/apache/polaris/blob/1f3023d5212bad53e53a5db9ce878d1d630901ec/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java#L223-L230
But my findings showed that this will not work in this case, and I believe it's because `CallContext` has a `close()` method that is called when the request processing ends:
https://github.com/apache/polaris/blob/4409252d135f62e9f9ae09d943c0c0f720c03efa/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java#L127-L129
Side note2: I also think the `CallContext.close()` method should be removed and the catalog-closing logic should be refactored. I will log an issue for that as soon as this PR is merged.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1964868862
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/task/QuarkusTaskExecutorImpl.java:
##########
@@ -39,16 +39,17 @@ public class QuarkusTaskExecutorImpl extends TaskExecutorImpl {
private final Tracer tracer;
public QuarkusTaskExecutorImpl() {
- this(null, null, null, null);
+ this(null, null, null, null, null);
}
@Inject
public QuarkusTaskExecutorImpl(
+ CallContext callContext,
Review Comment:
The fact that this class is `ApplicationScoped` should mean that it can't be constructed with a `CallContext` -- something is definitely very wrong here
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965144779
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -28,57 +27,47 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.polaris.core.PolarisConfigurationStore;
-import org.apache.polaris.core.PolarisDiagnostics;
-import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.TaskEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
-import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Given a list of registered {@link TaskHandler}s, execute tasks asynchronously with the provided
- * {@link RealmContext}.
+ * {@link CallContext}.
*/
public class TaskExecutorImpl implements TaskExecutor {
private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutorImpl.class);
private static final long TASK_RETRY_DELAY = 1000;
+ private final CallContext callContext;
private final Executor executor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
- private final PolarisConfigurationStore configurationStore;
- private final PolarisDiagnostics diagnostics;
private final TaskFileIOSupplier fileIOSupplier;
- private final Clock clock;
private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
public TaskExecutorImpl(
+ CallContext callContext,
Review Comment:
Yes,`CallContext` should not be injected but cloned. Here is how it was handled before:
https://github.com/apache/polaris/blob/f0c99a69936192f2e337c6f9ea17c7c3d1ccdccc/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java#L100
The reason is that `CallContext` being a request-scoped bean, it becomes tricky to pass it on to a different thread pool, as the instance might survive there longer than the HTTP request itself. Thus it becomes necessary to create a copy of it and pass the copy to the executor. (The copy must be a deep clone: `PolarisCallContext` _must_ be cloned too, especially because of the metastore session bean.)
Side note 1: Quarkus has context propagation techniques, and in fact, the `CallContext` bean would be properly propagated to the executor, because the executor is a Quarkus-managed executor:
https://github.com/apache/polaris/blob/1f3023d5212bad53e53a5db9ce878d1d630901ec/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java#L223-L230
But my findings showed that this will not work in this case, and I believe it's because `CallContext` has a `close()` method that is called when the request processing ends:
https://github.com/apache/polaris/blob/4409252d135f62e9f9ae09d943c0c0f720c03efa/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java#L127-L129
Side note2: I also think the `CallContext.close()` method should be removed and the catalog-closing logic should be refactored. I will log an issue for that as soon as this PR is merged.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1964877242
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -28,57 +27,47 @@
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
-import org.apache.polaris.core.PolarisConfigurationStore;
-import org.apache.polaris.core.PolarisDiagnostics;
-import org.apache.polaris.core.context.RealmContext;
+import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisBaseEntity;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityType;
import org.apache.polaris.core.entity.TaskEntity;
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
-import org.apache.polaris.core.persistence.PolarisMetaStoreSession;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Given a list of registered {@link TaskHandler}s, execute tasks asynchronously with the provided
- * {@link RealmContext}.
+ * {@link CallContext}.
*/
public class TaskExecutorImpl implements TaskExecutor {
private static final Logger LOGGER = LoggerFactory.getLogger(TaskExecutorImpl.class);
private static final long TASK_RETRY_DELAY = 1000;
+ private final CallContext callContext;
private final Executor executor;
private final MetaStoreManagerFactory metaStoreManagerFactory;
- private final PolarisConfigurationStore configurationStore;
- private final PolarisDiagnostics diagnostics;
private final TaskFileIOSupplier fileIOSupplier;
- private final Clock clock;
private final List<TaskHandler> taskHandlers = new CopyOnWriteArrayList<>();
public TaskExecutorImpl(
+ CallContext callContext,
Review Comment:
Since this basically is coming from `QuarkusTaskExecutorImpl` which can't possibly be injecting the correct CallContext, let's leave this out of the constructor for now. We need to figure out how to propagate the original call context for the `ManifestFileCleanupTaskHandler` and `TableCleanupTaskHandler` in different ways.
The pattern established here is that `addTaskHandlerContext` apparently is how we preserve a CallContext manually. So we just need to make sure that thing plumbs through.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1965155522
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -91,39 +80,45 @@ public void addTaskHandler(TaskHandler taskHandler) {
}
/**
- * Register a {@link RealmContext} for a specific task id. That task will be loaded and executed
- * asynchronously with a copy of the provided {@link RealmContext} (because the realm context is a
- * request-scoped component).
+ * Register a {@link CallContext} for a specific task id. That task will be loaded and executed
+ * asynchronously with a clone of the provided {@link CallContext}.
*/
@Override
- public void addTaskHandlerContext(long taskEntityId, RealmContext realmContext) {
- tryHandleTask(taskEntityId, RealmContext.copyOf(realmContext), null, 1);
+ public void addTaskHandlerContext(long taskEntityId, CallContext callContext) {
+ // Unfortunately CallContext is a request-scoped bean and must be cloned now,
+ // because its usage inside the TaskExecutor thread pool will outlive its
+ // lifespan, so the original CallContext will eventually be closed while
+ // the task is still running.
+ // Note: PolarisCallContext has request-scoped beans as well, and must be cloned.
+ // FIXME replace with context propagation?
+ CallContext clone = CallContext.copyOf(callContext);
+ tryHandleTask(taskEntityId, clone, null, 1);
}
private @Nonnull CompletableFuture<Void> tryHandleTask(
- long taskEntityId, RealmContext realmContext, Throwable e, int attempt) {
+ long taskEntityId, CallContext callContext, Throwable e, int attempt) {
if (attempt > 3) {
return CompletableFuture.failedFuture(e);
}
return CompletableFuture.runAsync(
- () -> handleTask(taskEntityId, realmContext, attempt), executor)
+ () -> handleTask(taskEntityId, callContext, attempt), executor)
.exceptionallyComposeAsync(
(t) -> {
LOGGER.warn("Failed to handle task entity id {}", taskEntityId, t);
- return tryHandleTask(taskEntityId, realmContext, t, attempt + 1);
+ return tryHandleTask(taskEntityId, callContext, t, attempt + 1);
},
CompletableFuture.delayedExecutor(
TASK_RETRY_DELAY * (long) attempt, TimeUnit.MILLISECONDS, executor));
}
- protected void handleTask(long taskEntityId, RealmContext realmContext, int attempt) {
+ protected void handleTask(long taskEntityId, CallContext ctx, int attempt) {
+ // set the call context INSIDE the async task
+ CallContext.setCurrentContext(ctx);
Review Comment:
Yes that was done before:
https://github.com/apache/polaris/blob/4409252d135f62e9f9ae09d943c0c0f720c03efa/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java#L110-L112
(We weren't clearing the context in a finally block though – but I don't think it's necessary since the thread local is being set at the very beginning of every task execution. It wouldn't hurt to add it though.)
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1000:
URL: https://github.com/apache/polaris/pull/1000#issuecomment-2658663914
The current build issues come from DI, e.g.:
```
Execution failed for task ':polaris-quarkus-server:quarkusAppPartsBuild'.
> There was a failure while executing work items
> A failure occurred while executing io.quarkus.gradle.tasks.worker.BuildWorker
> io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.arc.deployment.ArcProcessor#validate threw an exception: jakarta.enterprise.inject.spi.DeploymentException: Found 3 deployment problems:
[1] Unsatisfied dependency for type org.apache.polaris.core.persistence.PolarisEntityManager and qualifiers [@Default]
- injection target: parameter 'entityManager' of org.apache.polaris.service.catalog.IcebergCatalogAdapter constructor
- declared on CLASS bean [types=[org.apache.polaris.service.catalog.IcebergCatalogAdapter, org.apache.polaris.service.catalog.api.IcebergRestCatalogApiService, org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService, java.lang.Object], qualifiers=[@Default, @Any], target=org.apache.polaris.service.catalog.IcebergCatalogAdapter]
[2] Unsatisfied dependency for type org.apache.polaris.core.persistence.PolarisMetaStoreManager and qualifiers [@Default]
- injection target: parameter 'metaStoreManager' of org.apache.polaris.service.catalog.IcebergCatalogAdapter constructor
- declared on CLASS bean [types=[org.apache.polaris.service.catalog.IcebergCatalogAdapter, org.apache.polaris.service.catalog.api.IcebergRestCatalogApiService, org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService, java.lang.Object], qualifiers=[@Default, @Any], target=org.apache.polaris.service.catalog.IcebergCatalogAdapter]
[3] Unsatisfied dependency for type org.apache.polaris.core.persistence.PolarisMetaStoreSession and qualifiers [@Default]
- injection target: parameter 'session' of org.apache.polaris.service.catalog.IcebergCatalogAdapter constructor
- declared on CLASS bean [types=[org.apache.polaris.service.catalog.IcebergCatalogAdapter, org.apache.polaris.service.catalog.api.IcebergRestCatalogApiService, org.apache.polaris.service.catalog.api.IcebergRestConfigurationApiService, java.lang.Object], qualifiers=[@Default, @Any], target=org.apache.polaris.service.catalog.IcebergCatalogAdapter]
```
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dennishuo (via GitHub)" <gi...@apache.org>.
dennishuo commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1964882018
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java:
##########
@@ -91,39 +80,45 @@ public void addTaskHandler(TaskHandler taskHandler) {
}
/**
- * Register a {@link RealmContext} for a specific task id. That task will be loaded and executed
- * asynchronously with a copy of the provided {@link RealmContext} (because the realm context is a
- * request-scoped component).
+ * Register a {@link CallContext} for a specific task id. That task will be loaded and executed
+ * asynchronously with a clone of the provided {@link CallContext}.
*/
@Override
- public void addTaskHandlerContext(long taskEntityId, RealmContext realmContext) {
- tryHandleTask(taskEntityId, RealmContext.copyOf(realmContext), null, 1);
+ public void addTaskHandlerContext(long taskEntityId, CallContext callContext) {
+ // Unfortunately CallContext is a request-scoped bean and must be cloned now,
+ // because its usage inside the TaskExecutor thread pool will outlive its
+ // lifespan, so the original CallContext will eventually be closed while
+ // the task is still running.
+ // Note: PolarisCallContext has request-scoped beans as well, and must be cloned.
+ // FIXME replace with context propagation?
+ CallContext clone = CallContext.copyOf(callContext);
+ tryHandleTask(taskEntityId, clone, null, 1);
}
private @Nonnull CompletableFuture<Void> tryHandleTask(
- long taskEntityId, RealmContext realmContext, Throwable e, int attempt) {
+ long taskEntityId, CallContext callContext, Throwable e, int attempt) {
if (attempt > 3) {
return CompletableFuture.failedFuture(e);
}
return CompletableFuture.runAsync(
- () -> handleTask(taskEntityId, realmContext, attempt), executor)
+ () -> handleTask(taskEntityId, callContext, attempt), executor)
.exceptionallyComposeAsync(
(t) -> {
LOGGER.warn("Failed to handle task entity id {}", taskEntityId, t);
- return tryHandleTask(taskEntityId, realmContext, t, attempt + 1);
+ return tryHandleTask(taskEntityId, callContext, t, attempt + 1);
},
CompletableFuture.delayedExecutor(
TASK_RETRY_DELAY * (long) attempt, TimeUnit.MILLISECONDS, executor));
}
- protected void handleTask(long taskEntityId, RealmContext realmContext, int attempt) {
+ protected void handleTask(long taskEntityId, CallContext ctx, int attempt) {
+ // set the call context INSIDE the async task
+ CallContext.setCurrentContext(ctx);
Review Comment:
Instead of this, can we passthrough the CallContext to `boolean success = handler.handleTask(task);` below? Like
boolean success = handler.handleTask(task, callContext);
I guess that requires changing the signature but right now the underlying handler doesn't even use the threadlocal so this `setCurrentContext` doesn't help it at all. Do we know if anything downstream here actually does require the threadlocal?
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #1000:
URL: https://github.com/apache/polaris/pull/1000#issuecomment-2664141445
CI still suggests some issues with injection:
```
> Task :polaris-quarkus-service:test
PolarisAdminServiceAuthzTest > testAssignPrincipalRoleSufficientPrivileges() FAILED
java.lang.NullPointerException at BasePolarisCatalog.java:1616
```
However, locally the tasks look OK to me:
```
$ ./gradlew clean :polaris-quarkus-service:test --rerun-tasks --no-parallel --no-build-cache
BUILD SUCCESSFUL in 1m 6s
77 actionable tasks: 76 executed, 1 up-to-date
```
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revert "Remove CallContext and its ThreadLocal usage" [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1000:
URL: https://github.com/apache/polaris/pull/1000#discussion_r1964405389
##########
polaris-core/src/main/java/org/apache/polaris/core/context/CallContext.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.core.context;
+
+import jakarta.annotation.Nonnull;
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.iceberg.io.CloseableGroup;
+import org.apache.polaris.core.PolarisCallContext;
+import org.apache.polaris.core.PolarisDiagnostics;
+import org.apache.polaris.core.auth.AuthenticatedPolarisPrincipal;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Stores elements associated with an individual REST request such as RealmContext, caller
+ * identity/role, authn/authz, etc. This class is distinct from RealmContext because implementations
+ * may need to first independently resolve a RealmContext before resolving the identity/role
+ * elements of the CallContext that reside exclusively within the resolved Realm. For example, the
+ * principal/role entities may be defined within a Realm-specific persistence layer, and the
+ * underlying nature of the persistence layer may differ between different realms.
+ */
+public interface CallContext extends AutoCloseable {
+ InheritableThreadLocal<CallContext> CURRENT_CONTEXT = new InheritableThreadLocal<>();
Review Comment:
I'm ok with this revert, as long as we all agree to remove thread locals (again) after merging 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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org