Rename "RealmId" to "Realm" #899
Conversation
polaris-core/src/main/java/org/apache/polaris/core/context/Realm.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Value.Parameter | ||
| @JsonValue | ||
| String id(); | ||
| String name(); |
There was a problem hiding this comment.
I think id and name are almost interchangeable, but I change it to name to be consistent with other entities in Polaris, like catalog, namespace.
flyrain
requested review from
adutra,
ashvina,
dennishuo,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode and
ebyhr
as code owners
integration-tests/src/main/java/org/apache/polaris/service/it/ext/PolarisServerManager.java
Outdated
Show resolved
Hide resolved
| new PolarisMetaStoreManagerImpl(realmId, diagnostics, configurationStore, clock); | ||
| metaStoreManagerMap.put(realmId.id(), metaStoreManager); | ||
| new PolarisMetaStoreManagerImpl(realm, diagnostics, configurationStore, clock); | ||
| metaStoreManagerMap.put(realm.name(), metaStoreManager); |
There was a problem hiding this comment.
Can this be keyed on Realm instead of String?
There was a problem hiding this comment.
I was think of the refactor of this part, but it seems a relative big refactor as it changes the key type of the map. Can we do that as a followup PR?
There was a problem hiding this comment.
If it's a pretty big change we can defer it. My concern is the same as you noted in another comment, making everything consistent.
...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java
Show resolved
Hide resolved
...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java
Show resolved
Hide resolved
...est/java/org/apache/polaris/service/storage/azure/AzureCredentialStorageIntegrationTest.java
Show resolved
Hide resolved
|
Thanks @eric-maynard. Resolved the comments. Ready for another look. |
| @@ -53,7 +53,7 @@ public InMemoryStorageIntegration( | |||
| * Check that the locations being accessed are all equal to or subdirectories of at least one of | |||
| * the {@link PolarisStorageConfigurationInfo#getAllowedLocations}. | |||
| * | |||
| * @param realmId | |||
| * @param realm | |||
There was a problem hiding this comment.
nit: param without a description - might as well remove :)
There was a problem hiding this comment.
This change looks like it's implementing "Option 1" from #910, which says The interface can be extensible in case we want to add any subconcept to the realm. This is against the documented contract, which says Represents the ID of the realm used in a REST request ....
There is also a fundamental difference between the ID (or call it "name") of a realm and the attributes of a realm. This change seems to prepare the pathway add more attributes to the type that is solely meant to contain only the ID/name of the realm.
In any case, if realm-ID/name-related/custom information is needed, CDI mechanisms should be used to not affect other code.
Making Realm/Id extensible makes it even harder if not impossible for HTTP/REST layers components (realm-ID extraction, request throttling) to do their job properly and without unnecessary performance implications. Those "low level components" should really not need to have knowledge about any extension. This would not be the case with the proposed "option 1", where those would have to go though some code that likely does a lot of things that those components do not need.
Also, if Realm/Id is considered extensible, how should two extensions for "realm" from different sources work together - only one extension could win.
If it's not the case that this change is meant to add more attributes to Realm/Id, then this PR is a pure nomenclature change while changing class names and function signatures (aka a breaking change). Nomenclature is almost always subjective, but not worth such a change like this.
Therefore my -1 on this PR.
|
Closing after #920 |
To fix #910.