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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
