Revert "Remove CallContext and its ThreadLocal usage"

[eric-maynard]

This fully reverts #589, returning CallContext usage to its previous state before CDI

[dimas-b]

I'm ok with this revert, as long as we all agree to remove thread locals (again) after merging this PR.

[eric-maynard]

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.

[dennishuo]

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.

[adutra]

Yes,CallContext should not be injected but cloned. Here is how it was handled before:

polaris/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java

Lines 83 to 92 in 4409252

	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);
	}

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:

polaris/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java

Lines 223 to 230 in 1f3023d

	public ManagedExecutor taskExecutor(TaskHandlerConfiguration config) {
	return SmallRyeManagedExecutor.builder()
	.injectionPointName("task-executor")
	.propagated(ThreadContext.ALL_REMAINING)
	.maxAsync(config.maxConcurrentTasks())
	.maxQueued(config.maxQueuedTasks())
	.build();
	}

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:

polaris/quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java

Lines 127 to 129 in 4409252

	public void closeCallContext(@Disposes CallContext callContext) {
	callContext.close();
	}

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.

[adutra]

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 0a2f9df, which is the commit before the CallContext removal by b84f462.

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.

[eric-maynard]

No, it's definitely not a clean revert unfortunately. There were very many conflicts with the revert applied. Taking a look at TaskExecutorImpl.java

[dennishuo]

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.

[dennishuo]

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?

[dennishuo]

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.

[adutra]

Yes that was done before:

polaris/service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java

Lines 110 to 112 in 4409252

	protected void handleTask(long taskEntityId, CallContext ctx, int attempt) {
	// set the call context INSIDE the async task
	CallContext.setCurrentContext(ctx);

(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.)

[eric-maynard]

Changed per this comment -- good catch!

[dennishuo]

LGTM, thanks!