Skip to content
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

Jump to bottom

Extend FileIOFactory to Improve Customization #724

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Extend FileIOFactory to Improve Customization #724

wants to merge 9 commits into from
+699 −190

Conversation

XJDKC
Copy link
Member

@XJDKC XJDKC commented Jan 13, 2025 •
edited
Loading

Context

PolarisEntity has an internalProperties map.

Some PolarisEntity have StorageConfig, these storage configs are stored as internalProperties in the Entity object. Also the internalProperties may contain additional storage config injected by customized PolarisMetaStoreManager.

It's very useful to pass these properties to FileIO so that the FileIO can implement some custom logic based on it.

This PR updates the BasePolarisCatalog::refreshIOWithCredentials method to pass the internal properties of the storageInfoEntity to FileIO object.

Some code pointers:

@XJDKC XJDKC force-pushed the pass-internal-properties-to-fileio branch from 057bd17 to 836d3f1 Compare January 13, 2025 23:01
@XJDKC XJDKC changed the title Pass InternalProperties to FileIO Pass Internal Properties to FileIO Jan 13, 2025
@XJDKC XJDKC mentioned this pull request Jan 13, 2025
Open
@XJDKC XJDKC force-pushed the pass-internal-properties-to-fileio branch from 836d3f1 to 0a10632 Compare January 14, 2025 09:04
@snazy
Copy link
Member

snazy commented Jan 14, 2025

Thanks for your contribution @XJDKC!
I think this change blurs the line between "properties" and "internal properties" quite a bit. I'm also, in general, concerned about generic property bag per se containing all kinds of information - things that can be exposed (upstream/downstream) and things that must not - nobody can ensure that no "bad things" (think: logging secrets) happen to this information.

@XJDKC XJDKC force-pushed the pass-internal-properties-to-fileio branch from 88bc1af to 6ff8f2b Compare January 14, 2025 18:10
@dimas-b dimas-b mentioned this pull request Jan 17, 2025
Open
@XJDKC XJDKC changed the title Pass Internal Properties to FileIO Extend FileIOFactory to Improve Customization Jan 22, 2025
dimas-b
dimas-b reviewed Jan 22, 2025
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the update @XJDKC ! Overall, the new code looks good to me :) A few minor comments below.

service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOFactory.java Outdated Show resolved Hide resolved
snazy
snazy reviewed Jan 22, 2025
View reviewed changes
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated
|| storageActions.contains(PolarisStorageActions.ALL)
? tableLocations
: Set.of();
Map<String, String> credentialsMap =
Copy link
Member

Choose a reason for hiding this comment

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

absolute nitpick and totally my own personal preference: using Java's (new-ish) var keyword actually increases readability, assuming that the variable name is expressive.

service/common/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java Outdated Show resolved Hide resolved
...service/src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogTest.java Outdated
@@ -1459,6 +1462,46 @@ public void testDropTableWithPurgeDisabled() {
.hasMessageContaining(PolarisConfiguration.DROP_WITH_PURGE_ENABLED.key);
}

@Test
public void testRefreshIOWithCredentialsAndInternalProperties() {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this actually refreshes credentials.

Anyway, having this test is good. However, I'd prefer to also have a dedicated test class for DefaultFileIOFactory and do extensive testing there, also with multiple realms?

service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOFactory.java Outdated Show resolved Hide resolved
@XJDKC XJDKC force-pushed the pass-internal-properties-to-fileio branch from 00a9ae4 to decc5b8 Compare January 22, 2025 17:09
dennishuo
dennishuo approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@dennishuo dennishuo left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-rxing sfc-gh-rxing sfc-gh-rxing left review comments

@dimas-b dimas-b dimas-b left review comments

@snazy snazy snazy left review comments

@dennishuo dennishuo dennishuo approved these changes

@adutra adutra Awaiting requested review from adutra adutra is a code owner

@ashvina ashvina Awaiting requested review from ashvina ashvina is a code owner

@eric-maynard eric-maynard Awaiting requested review from eric-maynard eric-maynard is a code owner

@jackye1995 jackye1995 Awaiting requested review from jackye1995 jackye1995 is a code owner

@jbonofre jbonofre Awaiting requested review from jbonofre jbonofre is a code owner

@vvcephei vvcephei Awaiting requested review from vvcephei vvcephei is a code owner

@collado-mike collado-mike Awaiting requested review from collado-mike collado-mike is a code owner

@RussellSpitzer RussellSpitzer Awaiting requested review from RussellSpitzer RussellSpitzer is a code owner

@takidau takidau Awaiting requested review from takidau takidau is a code owner

@flyrain flyrain Awaiting requested review from flyrain flyrain is a code owner

@ebyhr ebyhr Awaiting requested review from ebyhr ebyhr is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@XJDKC @snazy @dennishuo @eric-maynard @collado-mike @dimas-b @sfc-gh-rxing