Remove PolarisMetaStoreSession from FileIOFactory/FileIOUtil in favor of CallContext #1057
Conversation
… of CallContext This appeared to be some leaky divergence that occurred after CallContext had been removed, but PolarisMetaStoreSession really is only a low-level implementation detail that should never be handled by BasePolarisCatalog/FileIOFactory. This plumbs CallContext explicitly into the FileIOFactory and FileIOUtil methods and thus removes a large source of CallContext.getCurrentContext calls; now the threadlocal doesn't have to be set at all in BasePolarisCatalogTest.
dennishuo
requested review from
MonkeyCanCode,
RussellSpitzer,
adutra,
ashvina,
collado-mike,
dimas-b,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
|
This looks like a more correct way to apply the revert and it removes the need for some threadlocal usage, which is great |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
It makes more sense to me and using CallContext (compared to RealContext) seems more consistent.
I agree that PolarisMetaStoreSession is a impl details and should not be used directly.
service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOFactory.java
Show resolved
Hide resolved
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
From my POV, it would be preferable to inject CallContext, but I understand that would be a larger change.
As an incremental change, in switching from RealmContext to CallContext in FileIOFactory my main concern is exposing PolarisMetaStoreSession.
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java
Show resolved
Hide resolved
| */ | ||
| FileIO loadFileIO( | ||
| @Nonnull RealmContext realmContext, | ||
| @Nonnull CallContext callContext, |
There was a problem hiding this comment.
This exposes PolarisMetaStoreSession (via PolarisCallContext). Do FileIO factories actually need PolarisCallContext?
There was a problem hiding this comment.
What part of CallContext is actually required by FileIO factories?
There was a problem hiding this comment.
to clarify: we're exposing extra information here. Removing what has been exposed is much harder than exposing more details later.
There was a problem hiding this comment.
Yeah it actually is already needed because it uses the getOrGenerateSubScopeCreds which for now is still in the main persistence interface (it's starting to be separated out via the PolarisCredentialVendor interface, but that still needs more work to fully split, and either way the Transacting impls need the MetaStoreSession to get the subscoped creds).
So it actually needs all of PolarisCallContext right now (and before this PR was accessing it via the ThreadLocal).
There was a problem hiding this comment.
That's a good point to keep in mind though that right now CallContext does inadvertently give a big bag of goodies to the methods being called, so we should still be careful to think about whether a smaller subset can be carved out to minimize surface area to what is actually used.
There was a problem hiding this comment.
getOrGenerateSubScopeCreds is defined in StorageCredentialCache not in PolarisMetaStoreSession.
I did not block this PR, but I think this API change is far from ideal.
@dennishuo : would you mind refactoring it some more to reduce the scope of services exposed to FileIO factories?
I agreed on inject, but suggest to make it happen in a separated PR. |
* Publish proposals and roadmap on the website (apache#1043) * Allow Polaris CLI to set property values that contain '=' (apache#1003) Currently the CLI parsing is too pessimistic and throws an error if it detects '=' in the parsed value of a property. However, the split 1 semantics are already standard behavior for most parsers where the right-hand-side is allowed to contain '='. This commonly comes up if the value is itself a comma-separated kv list or if it the value is a base64-encoded string. * main: Update postgres Docker tag to v17.4 (apache#1049) * main: Update dependency org.junit:junit-bom to v5.12.0 (apache#1045) * Revert "Remove CallContext and its ThreadLocal usage" (apache#1000) * revert b84f462 * resolve conflicts * autolint * autolint * ...not possible to automatically add a synthetic no-args constructor to an unproxyable bean class. * fix * fixed non-quarkus build issues * fixed non-quarkus build issues * autolint * more di issues * add more producers * add another producer * autolint * change callcontext usage in FileIOFactoryTest * stablize tests * autolint * fix a test * refactor TableCleanupTaskHandlerTest * another fix, still npe * autolint * only task cleanup is problematic * autolint * Stabilize more tests * autolint * Avoid propagating old callcontexts across TestServices instances * testservices update * autolint * Revert "autolint" This reverts commit 66a7f22. * Revert "testservices update" This reverts commit 897a5a1. * changes to taskexecutorimpl * autolint --------- Co-authored-by: Dennis Huo <[email protected]> * Enable console color when running Polaris with Gradle (apache#977) * CLI: add subcommand access for principals (apache#1019) * Access subcommand access for principals * Access subcommand access for principals * Fix getting start spark (apache#1054) * Use correct link format for adoc and update menu title for proposals (apache#1051) * Add a debug log with the full exception (apache#1023) * clean up * spotless * clean up * refactor message * no CVEM * change log levels * revert polaris-java dep change * add testImplementation * uppercase * Remove all tightly coupled EntityCache dependencies in the main persistence stack (apache#1055) Remove the EntityCacheEntry wrapper since the timestamp fields were never used anyways; instead the underlying Caffeine cache transparently handles access times, and the types of entries we cache are simply the existing ResolvedPolarisEntity. Remove interactions of business logic with explicit "cache entries", instead operating on ResolvedPolarisEntity. Fix the equals()/hashCode() behavior of PolarisEntity to be compatible with PolarisBaseEntity as intended. Improve code comments to explain the (current) relationship between PolarisEntity and PolarisBaseEntity, and clarify the behaviors in Resolver.java. Fully remove the PolarisRemoteCache interface and its methods. Add different methods that aren't cache-specific instead. * Remove PolarisMetaStoreSession from FileIOFactory/FileIOUtil in favor of CallContext (apache#1057) This appeared to be some leaky divergence that occurred after CallContext had been removed, but PolarisMetaStoreSession really is only a low-level implementation detail that should never be handled by BasePolarisCatalog/FileIOFactory. This plumbs CallContext explicitly into the FileIOFactory and FileIOUtil methods and thus removes a large source of CallContext.getCurrentContext calls; now the threadlocal doesn't have to be set at all in BasePolarisCatalogTest. * Cleanup Iceberg Manifest list files on Purge (apache#1039) * Fix small quickstart typo (apache#1061) * Enable rat check on md files and fix the existing ones (apache#1050) * main: Update dependency boto3 to v1.37.0 (apache#1052) * main: Update dependency software.amazon.awssdk:bom to v2.30.27 (apache#1053) * Integration tests: improve support for temporary folders (apache#987) 3 integration tests currently require a temporary folder for various purposes. The folders are generally hard-coded to some local filesystem location, which makes those tests impossible to execute against a remote Polaris instance. This PR aims at fixing this situation by leveraging a new optional environment variable that points to a location that is accessible by both the server and the client running the tests. This would typically be an S3 bucket, for example, but could also be a shared volume mounted on both machines at the same mount point. If this environment variable is not present, then local execution is assumed and a temporary directory managed by JUnit is used instead. * build-logic: exclude generated code from errorprone (apache#1035) * build-logic: let javadoc depend on jandex (apache#1034) * Add Prashant in the collaborators list (apache#1064) * main: Update dependency org.slf4j:slf4j-api to v2.0.17 (apache#1065) --------- Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Dennis Huo <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: MonkeyCanCode <[email protected]> Co-authored-by: Andrew Guterman <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Robert Stupp <[email protected]>
6 participants
This appeared to be some leaky divergence that occurred after CallContext had been removed, but PolarisMetaStoreSession really is only a low-level implementation detail that should never be handled by BasePolarisCatalog/FileIOFactory.
This plumbs CallContext explicitly into the FileIOFactory and FileIOUtil methods and thus removes a large source of CallContext.getCurrentContext calls; now the threadlocal doesn't have to be set at all in BasePolarisCatalogTest.