adutra commented on code in PR #1783:
URL: https://github.com/apache/polaris/pull/1783#discussion_r2123027837
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -307,7 +308,10 @@ EntityResult loadEntity(
*/
@Nonnull
EntitiesResult loadTasks(
- @Nonnull PolarisCallContext callCtx, String executorId, PageToken
pageToken);
+ @Nonnull PolarisCallContext callCtx,
Review 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`.
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java:
##########
@@ -166,28 +154,24 @@ void testGetConfigurationWithRealm() {
@Test
public void testInjectedConfigurationStore() {
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
// the default value for trueByDefaultKey is `true`
boolean featureDefaultValue =
Review Comment:
nit: my IDE is warning me about nullability issues, since the return types
are marked `@Nullable` :-)
The following change removes the warnings:
```java
Boolean featureDefaultValue =
configurationStore.getConfiguration(realmContext, trueByDefaultKey);
assertThat(featureDefaultValue).isTrue();
```
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java:
##########
@@ -97,51 +96,40 @@ public void before(TestInfo testInfo) {
Clock.systemDefaultZone());
}
Review Comment:
The following fields are now unused and should be completely removed:
```java
private PolarisCallContext polarisContext;
@Inject MetaStoreManagerFactory managerFactory;
@Inject PolarisDiagnostics diagServices;
```
##########
polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java:
##########
@@ -37,48 +36,6 @@
public interface PolarisConfigurationStore {
Review Comment:
This class is a bit inconsistent wrt nullability: sometimes the
`RealmContext` parameter is annotated with `@Nonnull`, sometimes it isn't. What
is the expectation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
