Remove CallContext.copy()
#2294
Conversation
The function is only needed to create a "safe clone" of `PolarisCallContext` in `TaskExecutorImpl`, which doesn't have references to CDI request scoped beans. To still let `TaskExecutorImpl` making "safe clones", a functionality to get (fresh) instances of `RealmContext` is required. To enable this, the `RealmContextResolver` has been enhanced with "`RealmContext` lookups" by realm-ID. That in turn led to splitting the HTTP/REST-to-realm-context resolution into two parts: HTTP/REST-to-realm-ID and realm-ID-to-context. This change is part of the effort to allow tasks run run in different JVMs.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| tryHandleTask(taskEntityId, new TaskContext(callContext), null, 1); | ||
| } | ||
|
|
||
| record TaskContext(String realmId, PolarisDiagnostics diagnostics) { |
There was a problem hiding this comment.
have we done some testing (at least manually to verify the task is still executing correctly)? as far as I know, we currently do not have a good integration test that actually helps verify the background task. The regtests t_pyspark/test_spark_sql_s3_with_privileges.py contains a tests that helps verify the background purge task, but not running in the CI today.
Can we follow up the readme here https://github.com/apache/polaris/blob/main/regtests/README.md to run the test against aws to verify things are still working?
There was a problem hiding this comment.
There was a problem hiding this comment.
CDI context propagation does not work across different JVMs for the tasks-proposals.
There was a problem hiding this comment.
have we done some testing
I mean, sure? There are tests in the code base for this.
The regtests t_pyspark/test_spark_sql_s3_with_privileges.py ... not running in the CI today.
I'd suggest to fix that independently. Mind taking a stab on that, @gh-yzou ? MinIO, Azurite and Google emulator help there.
There was a problem hiding this comment.
I mean, sure? There are tests in the code base for this.
Our task tests are not very complete right now, so this doesn't give me much confidence.
There was a problem hiding this comment.
t_pyspark/test_spark_sql_s3_with_privileges.py can only be run locally with your avaiable S3 account, @jbonofre is working on getting an S3 account for cli testing.
However, I am not sure what would be the extra benefit this approach would give us. The copy() method is used to propagate the whole call Context and RealmContext to the task execution, and the id look up seems also only used by task execution and requires an extra re-construction step. Furthermore, this approach doesn't seem scalable in the future if we are adding more information to callContext or RealmContext (such as user specific information), especially for RealmContext, it may require us to implement a RealmContextManager for lookup.
If the concern is that this function is only used by task executor, i recall @dimas-b was POC something that leverages CDI feature to propagate the CallContext to background thread, which seems a cleaner way to do that. @dimas-b maybe we can resume that work?
The function is only needed to create a "safe clone" of
PolarisCallContextinTaskExecutorImpl, which doesn't have references to CDI request scoped beans.To still let
TaskExecutorImplmaking "safe clones", a functionality to get (fresh) instances ofRealmContextis required. To enable this, theRealmContextResolverhas been enhanced with "RealmContextlookups" by realm-ID. That in turn led to splitting the HTTP/REST-to-realm-context resolution into two parts: HTTP/REST-to-realm-ID and realm-ID-to-context.This change is part of the effort to allow tasks run run in different JVMs.