Skip to content

Turn CallContext.copyOf into an interface instead of static function #1816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
flyrain merged 6 commits into from
Jun 5, 2025
Merged

Turn CallContext.copyOf into an interface instead of static function #1816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b68d329
remove copyof
gh-yzou Jun 4, 2025
15bd240
update code
gh-yzou Jun 5, 2025
6e263e9
add comment
gh-yzou Jun 5, 2025
a471b0b
use sustemUTC
gh-yzou Jun 5, 2025
c00bf5c
update clock usage
gh-yzou Jun 5, 2025
f47582f
address comments
gh-yzou Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,17 @@ public RealmContext getRealmContext() {
public PolarisCallContext getPolarisCallContext() {
return this;
}

@Override
public PolarisCallContext copy() {
// The realm context is a request scoped bean injected by CDI,
// which will be closed after the http request. This copy is currently
// only used by TaskExecutor right before the task is handled, since the
// task is executed outside the active request scope, we need to make a
// copy of the RealmContext to ensure the access during the task executor.
String realmId = this.realmContext.getRealmIdentifier();
RealmContext realmContext = () -> realmId;
return new PolarisCallContext(
realmContext, this.metaStore, this.diagServices, this.configurationStore, this.clock);
Comment on lines +100 to +108

@dimas-b dimas-b Jun 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic of the comment makes sense to me in general.

However, I do not think it matters in practice ATM, because RealmContext implementations are not really closable. Those objects are quite usable (via latent references) even after the associated request context is destroyed.

This PR LGTM as an incremental improvement. In the future, I'd like to manage all contextual data via injection (automated in Quarkus CDI runtime or manual in customized env.)

@adutra adutra Jun 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those objects are quite usable (via latent references) even after the associated request context is destroyed.

I am not entirely sure: if the bean is @ApplicationScoped then a proxy is created for it. This means that calling RealmContext.getRealmIdentifier() will fail if the context has been destroyed.

If however the bean is @Singleton, I believe that yes, the bean would remain usable after the context is destroyed, because there is no proxy created for it.

This begs the question: should we use @Singleton whenever possible?

@dimas-b dimas-b Jun 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about proxies 👍 I stand corrected 🤦

@dimas-b dimas-b Jun 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Veering away from @Singleton. I'd like to avoid copying / cloning contexts completely and rely on CDI for injection in Quarkus. The open question here is how to deal with custom downstream builds that use polaris-core classes directly. I hope we can refactor that to allow for reuse via constructors and factories.

}
}
25 changes: 1 addition & 24 deletions polaris-core/src/main/java/org/apache/polaris/core/context/CallContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,30 +45,7 @@ static void unsetCurrentContext() {
}

/** Copy the {@link CallContext}. */
static CallContext copyOf(CallContext base) {
String realmId = base.getRealmContext().getRealmIdentifier();
RealmContext realmContext = () -> realmId;
PolarisCallContext originalPolarisCallContext = base.getPolarisCallContext();
PolarisCallContext newPolarisCallContext =
new PolarisCallContext(
realmContext,
originalPolarisCallContext.getMetaStore(),
originalPolarisCallContext.getDiagServices(),
originalPolarisCallContext.getConfigurationStore(),
originalPolarisCallContext.getClock());

return new CallContext() {
@Override
public RealmContext getRealmContext() {
return realmContext;
}

@Override
public PolarisCallContext getPolarisCallContext() {
return newPolarisCallContext;
}
};
}
CallContext copy();

RealmContext getRealmContext();

Expand Down
2 changes: 1 addition & 1 deletion service/common/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public void addTaskHandlerContext(long taskEntityId, CallContext callContext) {
// 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);
CallContext clone = callContext.copy();
tryHandleTask(taskEntityId, clone, null, 1);
}

Expand Down
25 changes: 6 additions & 19 deletions service/common/src/test/java/org/apache/polaris/service/catalog/io/FileIOFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,25 +136,12 @@ FileIO loadFileIOInternal(
.build();

callContext =
new CallContext() {
@Override
public RealmContext getRealmContext() {
return testServices.realmContext();
}

@Override
public PolarisCallContext getPolarisCallContext() {
return new PolarisCallContext(
realmContext,
testServices
.metaStoreManagerFactory()
.getOrCreateSessionSupplier(realmContext)
.get(),
testServices.polarisDiagnostics(),
testServices.configurationStore(),
Mockito.mock(Clock.class));
}
};
new PolarisCallContext(
realmContext,
testServices.metaStoreManagerFactory().getOrCreateSessionSupplier(realmContext).get(),
testServices.polarisDiagnostics(),
testServices.configurationStore(),
Clock.systemUTC());
}

@AfterEach
Expand Down
22 changes: 6 additions & 16 deletions service/common/src/testFixtures/java/org/apache/polaris/service/TestServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,22 +163,12 @@ public TestServices build() {
BasePersistence metaStoreSession =
metaStoreManagerFactory.getOrCreateSessionSupplier(realmContext).get();
CallContext callContext =
new CallContext() {
@Override
public RealmContext getRealmContext() {
return realmContext;
}

@Override
public PolarisCallContext getPolarisCallContext() {
return new PolarisCallContext(
realmContext,
metaStoreSession,
polarisDiagnostics,
configurationStore,
Mockito.mock(Clock.class));
}
};
new PolarisCallContext(
realmContext,
metaStoreSession,
polarisDiagnostics,
configurationStore,
Clock.systemUTC());
PolarisEntityManager entityManager =
realmEntityManagerFactory.getOrCreateEntityManager(realmContext);
PolarisMetaStoreManager metaStoreManager =
Expand Down
Loading