Replace getConfiguration usage with PolarisCallContext to use RealmContext (PART 1) #1780
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gh-yzou
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
adutra,
ajantha-bhat,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
pingtimeout,
singhpk234,
snazy,
takidau and
vvcephei
as code owners
Contributor
Author
flyrain
reviewed
Jun 2, 2025
Comment on lines
-81
to
+85
| .flatMap(c -> Optional.ofNullable(c.getPolarisCallContext())) | ||
| .map( | ||
| pc -> | ||
| pc.getConfigurationStore() | ||
| .getConfiguration(pc, "ALLOW_WILDCARD_LOCATION", false)) | ||
| ctx -> | ||
| ctx.getPolarisCallContext() | ||
| .getConfigurationStore() | ||
| .getConfiguration(ctx.getRealmContext(), "ALLOW_WILDCARD_LOCATION", false)) |
Contributor
There was a problem hiding this comment.
I think so, because we need to "undo" the translation from CallContext to PolarisCallContext so that we can recover the RealmContext.
Contributor
There was a problem hiding this comment.
My bad, didn't notice there was a pc :-)
flyrain
approved these changes
Jun 2, 2025
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
eric-maynard
approved these changes
Jun 2, 2025
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
dimas-b
reviewed
Jun 2, 2025
| */ | ||
| default <T> @Nonnull T getConfiguration( | ||
| PolarisCallContext ctx, | ||
| RealmContext realmContext, |
Contributor
There was a problem hiding this comment.
nit: why not keep the old param name?
Contributor
Author
There was a problem hiding this comment.
i think realmContext is a better parameter name that indicates clearly what the parameter is. "ctx" is very confusing by during the read of code like whether it is for CallContext or PolarisCallContext.
| Assertions.assertEquals("entity-new3", store.getConfiguration(ctx, entity, cfg.apply(3))); | ||
| Assertions.assertEquals("entity-legacy4", store.getConfiguration(ctx, entity, cfg.apply(4))); | ||
| Assertions.assertEquals( | ||
| "test-default1", store.getConfiguration(testRealmContext, entity, cfg.apply(1))); |
Contributor
There was a problem hiding this comment.
testRealmContext is not relevant here and does not affect the test's behaviour. I'd prefer to keep null.
Contributor
Author
There was a problem hiding this comment.
sg! I will update this in my part 2 PR.
Contributor
|
This PR is related to #1775 |
Contributor
Contributor
|
Sorry for the auto-merging, hopefully the part 2 can address your comments. cc @gh-yzou. |
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Jun 13, 2025
* Fix regression test docker setup for purge (apache#1768) * Use canonical catalog property names in tests (apache#1766) * In `PolarisPolicyServiceIntegrationTest` * In `PolarisRestCatalogIntegrationTest` Following up to apache#1557 * Unblock test `createViewWithCustomMetadataLocation` (apache#1320) * Add test for invalid custom metadata location * Add missing properties during table/view creation * main: Update docker.io/prom/prometheus Docker tag to v3.4.1 (apache#1767) * Testing: silence a bunch of harmless test warnings (apache#1773) * `OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended` * Hibernate Validator cannot instrument static methods (`Hibernate Validator does not support constraints on static methods yet. ...`) * ForkJoinPool test lifecycle warning * Couple of split-package warnings * Add unit test for legacy config lookup (apache#1774) Following up on apache#1766 * Handle RequestScoped instance injection gracefully for DefaultConfigurationStore (apache#1758) Although we do a check of !realmContextInstance.isUnsatisfied() it only checks if there is matching bean, however, it doesn't check if the context is active or not. Similarly isResolvable also don't check if the scope is active. Therefore if getConfiguration is called when there is no active scope, it will get Exception like ContextNotActiveException. This actually fails the TaskExecutor because it runs in a separate thread. In order to fix the problem, we introduces a new getConfiguration function to handle the background tasks, and also use isResolvable instead of isUnsatisfied to handle ambiguities. * Restructure the directory and package name for persistence modules (apache#1724) * Re-add missing parameters to create_table python API (apache#1778) It looks like this method lost the `prefix` and `namespace` parameters in apache#1347. They're re-introduced to the spec here, and then I've run: ``` redocly bundle spec/polaris-catalog-service.yaml -o spec/generated/bundled-polaris-catalog-service.yaml ./gradlew regeneratePythonClient ``` Then, some manual reverts: ``` alias gitrevert='git checkout upstream/main --' gitrevert client/python/.github/workflows/python.yml gitrevert client/python/.gitlab-ci.yml gitrevert client/python/pyproject.toml ``` I still hope to automate this process as part of CI soon; see apache#1675 * Replace getConfiguration usage with PolarisCallContext to use RealmContext (PART 1) (apache#1780) * JDBC: Fix getting started config (apache#1781) Fix typo in the JDBC config for getting started, the config should be max_duration_in_ms instead of max_delay_in_ms * main: Pin dependencies (apache#1701) * main: Update dependency pytest to v8 (apache#1710) * main: Update dependency boto3 to v1.38.28 (apache#1777) * Run renovatebot only on the main branch (apache#1786) * INFO: last merged commit a827d26 --------- Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Liam Bao <liam.zw.bao@gmail.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: Yufei Gu <yufei@apache.org> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: JB Onofré <jbonofre@apache.org>
travis-bowen
pushed a commit
to travis-bowen/polaris
that referenced
this pull request
Jun 20, 2025
* Replace getConfiguration usage with PolarisCallContext to use RealmContext (PART 1) (apache#1780) * cherry pick --------- Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
getConfiguration should be called per RealmContext instead of per PolarisCallContext. In this PR #1758 we introduced new API to that passes RealmContext, instead. of PolarisCallContext.
This PR refactors all calls except the loadTasks to use the new API with RealmContext, instead of calling with PolarisCallContext.
Follow up PR to update the usage in loadTasks and completely remove the getConfiguration function that uses PolarisCallContext