You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "snazy (via GitHub)" <gi...@apache.org> on 2026/02/16 16:12:45 UTC
[PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
snazy opened a new pull request, #3788:
URL: https://github.com/apache/polaris/pull/3788
Remove the unnecessary "no entity cache" test class, which is not relevant for production workloads.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2822837895
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
It is doable in code, though... NoSQL Persistence runs without an `EntityCache`, so other custom impl. may elect to run without it too. My point is that it's probably worth testing it in the general case, not only via NoSQL Persistence.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2818130181
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
I believe testing without the `EntityCache` is still relevant since the impl. of `EntityCache` may not always be present (the system can be set up without it). Therefore, I think it makes sense to validate that Polaris still works without it.
NoSQL test probably cover that use case to some extent, but it is still possible with JDBC. I suppose it is not unrealistic to run JDBC Persistence without `EntityCache` with (hypothetically) some other Persistence-level cache.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2822981981
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
I still think the "no entity cache" tests make sense even with JDBC Persistence because the cache is exercised in the core code, which is supposed to work with any persistence, but I do not insist.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2822955993
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
The option to run with no entity-cache was introduced for NoSQL.
> other custom impl. may elect
It's rather hypothetical. Any custom implementation would require its own tests anyway.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2813518087
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -293,32 +278,6 @@ public void before(TestInfo testInfo) {
QuarkusMock.installMockForType(realmContext, RealmContext.class);
Review Comment:
You probably could replace this mock with:
```java
@Inject RealmContextHolder realmContextHolder;
@BeforeEach
public void before(TestInfo testInfo) {
...
RealmContext realmContext = () -> realmName;
realmContextHolder.set(realmContext);
}
```
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2822856423
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
The "no entity cache" tests were added in ##1149 specifically to ensure core code correct in that situation.
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
The "no entity cache" tests were added in #1149 specifically to ensure core code correct in that situation.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2821021813
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
Not sure, there's no way to configure Polaris to run without that cache.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2822837895
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
It is doable in code, though... NoSQL Persistence runs without an `EntityCache`, so other customer impl. may elect to run without it too. My point is that it's probably worth testing it in the general case, not only via NoSQL Persistence.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3788:
URL: https://github.com/apache/polaris/pull/3788#discussion_r2822856423
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogTest.java:
##########
@@ -289,36 +275,9 @@ public void before(TestInfo testInfo) {
testInfo.getTestMethod().map(Method::getName).orElse("test"), System.nanoTime());
bootstrapRealm(realmName);
- RealmContext realmContext = () -> realmName;
- QuarkusMock.installMockForType(realmContext, RealmContext.class);
+ realmContextHolder.set(() -> realmName);
polarisContext = callContext.getPolarisCallContext();
- EntityCache entityCache = createEntityCache(diagServices, realmConfig, metaStoreManager);
Review Comment:
The "no entity cache" tests were added in #1149 specifically to ensure core code correctness in that situation.
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use more CDI in AbstractIcebergCatalogTest [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy merged PR #3788:
URL: https://github.com/apache/polaris/pull/3788
--
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-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org