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

gh-yzou · 2025-06-05T01:30:54Z

We want to turn CallContext class into a pure interface that can be implemented/customized by its child class. This PR removes the copyOf static function.

HonahX

LGTM!

dimas-b · 2025-06-05T02:17:11Z

service/common/src/testFixtures/java/org/apache/polaris/service/TestServices.java

+              metaStoreSession,
+              polarisDiagnostics,
+              configurationStore,
+              Mockito.mock(Clock.class));


nit: Clock.systemUTC() is simpler.

sg! updated

flyrain · 2025-06-05T02:42:11Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java

+
+  @Override
+  public PolarisCallContext copy() {
+    // make a copy of the realm context


nit: can we explain why we need a realm context copy here?

Yes, I was wondering about that too :)

One this that irks me is that a RealmContext impl. may not be immutable. We do not know that here. However, it's a minor point, IMHO.

I think I knew why, but a comment was supposed to explain why instead of what. It's easier to know what we do here by reading the code, but harder to know why we do that.

yes, added, please take a look and see if it make sense

The added text LGTM 👍

adutra · 2025-06-05T12:03:03Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java

  }
+
+  @Override
+  public CallContext copy() {


nit:

Suggested change

public CallContext copy() {

public PolarisCallContext copy() {

dimas-b · 2025-06-05T14:34:12Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java

+    // 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);


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

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?

Good point about proxies 👍 I stand corrected 🤦

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.

* main: Update dependency software.amazon.awssdk:bom to v2.31.54 (apache#1769) * main: Update dependency org.apache.spark:spark-sql_2.12 to v3.5.6 (apache#1752) * main: Update docker.io/apache/spark Docker tag to v3.5.6 (apache#1795) * main: Update dependency org.apache.spark:spark-sql_2.12 to v3.5.6 (apache#1794) * main: Update apache/spark Docker tag to v3.5.6 (apache#1791) * feat(cdi): Remove CallContext.close() (apache#1776) Now that every catalog created by PolarisCallContextCatalogFactory is correctly closed by IcebergCatalogAdapter, we can finally remove the CallContext.close() method and the associated cloaseables group. This simplification will hopefully pave the way to a more robust handling of request-scoped beans in task executor threads. * Remove the unused field in CallContextCatalogFactory (apache#1784) * Test: silence CDS warning from admin tool tests (apache#1800) * Update ascii banner (apache#1654) * Don't rotate root's credentials on startup in JdbcMetaStoreManagerFactory (apache#1804) * Core: Consolidate CallContext with PolarisCallContext part 1(apache#1806) * Automate regeneration of Python client (apache#1675) When running regtests or the CLI, we should run `./gradlew regeneratePythonClient` in order to run against the most up-to-date Python client. As part of this change, I've also updated the generator to 7.12 to match what's in the `libs.version.toml` and regenerated the code managed in github. Once the automatic generation works as part of CI, we can remove the generated code from the repo altogether. In the future, it can be added to .gitignore. * Remove deprecated constructor for PolarisCallContext (apache#1813) After pr apache#1806, the original constructor is marked as deprecated and is not suppose to be used anymore. This PR removes the usage of the old constructors. * Remove CallContext.of (apache#1812) This PR is a follow-up of apache#1806 to remove all usage of CallContext.of() in tests and the method itself * Remove CallContext.getDiagnostics (apache#1815) This shall be part of the CallContext cleanup process. `CallContext.getDiagnostics` is never used anywhere * Turn CallContext.copyOf into an interface instead of static function (apache#1816) * Refactor getConfiguration to use RealmContext (Part 2) (apache#1783) * main: Update dependency gradle to v8.14.2 (apache#1820) * main: Update dependency gradle to v8.14.2 * adjust to Polaris build --------- Co-authored-by: Dmitri Bourlatchkov <[email protected]> * NoSQL: merge-conflict fixes * Last merged commit: 553b644 --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Scott Teal <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: gh-yzou <[email protected]> Co-authored-by: Honah (Jonas) J. <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]>

gh-yzou added 2 commits June 4, 2025 18:07

remove copyof

b68d329

update code

15bd240

gh-yzou requested review from adutra, ashvina and dennishuo as code owners June 5, 2025 01:30

github-project-automation bot added this to Basic Kanban Board Jun 5, 2025

gh-yzou requested review from MonkeyCanCode, RussellSpitzer, collado-mike, dimas-b, ebyhr, eric-maynard, flyrain, jackye1995, jbonofre, snazy, takidau and vvcephei as code owners June 5, 2025 01:30

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jun 5, 2025

gh-yzou requested review from HonahX, ajantha-bhat, pingtimeout and singhpk234 as code owners June 5, 2025 01:30

HonahX previously approved these changes Jun 5, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 5, 2025

dimas-b previously approved these changes Jun 5, 2025

View reviewed changes

flyrain reviewed Jun 5, 2025

View reviewed changes

flyrain previously approved these changes Jun 5, 2025

View reviewed changes

add comment

6e263e9

gh-yzou dismissed flyrain’s stale review via 6e263e9 June 5, 2025 05:46

gh-yzou dismissed stale reviews from dimas-b and HonahX via 6e263e9 June 5, 2025 05:46

use sustemUTC

a471b0b

gh-yzou requested review from HonahX, dimas-b and flyrain June 5, 2025 05:58

update clock usage

c00bf5c

flyrain previously approved these changes Jun 5, 2025

View reviewed changes

adutra reviewed Jun 5, 2025

View reviewed changes

dimas-b reviewed Jun 5, 2025

View reviewed changes

dimas-b previously approved these changes Jun 5, 2025

View reviewed changes

address comments

f47582f

gh-yzou dismissed stale reviews from dimas-b and flyrain via f47582f June 5, 2025 16:50

singhpk234 approved these changes Jun 5, 2025

View reviewed changes

adutra approved these changes Jun 5, 2025

View reviewed changes

flyrain merged commit 187d700 into apache:main Jun 5, 2025
9 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jun 5, 2025

	public CallContext copy() {
	public PolarisCallContext copy() {

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

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

Uh oh!

Conversation

gh-yzou commented Jun 5, 2025

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dimas-b Jun 5, 2025 •

edited

Loading

dimas-b Jun 5, 2025 •

edited

Loading