Refactor getConfiguration to use RealmContext (Part 2)#1783
Refactor getConfiguration to use RealmContext (Part 2)#1783flyrain merged 6 commits intoapache:mainfrom
Conversation
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
There was a problem hiding this comment.
@dimas-b I added the change here to address your comment in the other PR #1780 (comment)
There was a problem hiding this comment.
This looks functionally correct to me, but it contradicts the @Nonnull annotation on the realm context parameter now 🤷 Let's go back to testRealmContext to be aligned with annotations.
There was a problem hiding this comment.
I wonder if it wouldn't be safer to pass CallContext otherwise you could in theory pass a PolarisCallContext that does not correspond to the passed RealmContext.
There was a problem hiding this comment.
There was a problem hiding this comment.
PolarisCallContext and RealmContext together in the same method call do raise an alarm in my head ;)
This is what I call "API skew" - both parameters are associated with a realm, but there's no guarantee the realm ID is the same. Moreover, there's no clarity what implementations should use for getting the realm ID.
CallContext might be acceptable in the short term if the ripple effects of using it are not too large.
In general, I'd like Polaris code to get all "context" stuff injected via CDI, though.
There was a problem hiding this comment.
To be clear: I'm -1 on using PolarisCallContext and RealmContext together in the same method.
+0 on using CallContext instead.
There was a problem hiding this comment.
Also for context: I personally do not have a good understanding of how PolarisCallContext and CallContext were envisioned to be used initially. My comments are based on what exists in code ATM.
There was a problem hiding this comment.
+1 on using CallContext. TBH, I'd be fine to replace all of the PolarisCallContext params in this interface with CallContext.
There was a problem hiding this comment.
+1 on using CallContext. More broadly, I'd consider consolidating CallContext and PolarisCallContext into one class. Any concerns?
There was a problem hiding this comment.
I'm fine with consolidating CallContext and PolarisCallContext into one class... but I reserve the right to object when I see the specific change set for that :) Note that ATM the former is an interface, the latter a class 🤷
polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java
Outdated
Show resolved
Hide resolved
...e/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
nit: my IDE is warning me about nullability issues, since the return types are marked @Nullable :-)
The following change removes the warnings:
Boolean featureDefaultValue =
configurationStore.getConfiguration(realmContext, trueByDefaultKey);
assertThat(featureDefaultValue).isTrue();
|
@dimas-b Thanks a lot for the heads up! I am aware of that change, and I am currently helping on some of the follow up work to position the whole polaris call context is a cleaner way, once finished will rebase the change. |
gh-yzou
force-pushed
the
yzou-get-configuration-refactor2
branch
from
851ccec to
7bdfaaf
Compare
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
There was a problem hiding this comment.
This looks functionally correct to me, but it contradicts the @Nonnull annotation on the realm context parameter now 🤷 Let's go back to testRealmContext to be aligned with annotations.
gh-yzou
force-pushed
the
yzou-get-configuration-refactor2
branch
from
ef9ff35 to
1179e98
Compare
|
@flyrain : over to you for review (just to be in sync) |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
* 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]>
5 participants
This is the last PR to deal with issue #1775.
This PR refactors loadTasks to take RealmContext as a parameter, and completely removes the usage of getConfiguration(@nonnull PolarisCallContext ctx, String configName).