Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3048513288
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
@flyrain : I took the liberty and followed up with #4136
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain merged PR #4091: URL: https://github.com/apache/polaris/pull/4091 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3040433566
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
ok, fair enough
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3040403961
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
I can do that as a followup.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3039677944
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
from my POV making `createCallContextCatalog()` return an `IcebergCatalog`
will complete the picture of making the logic more explicit in this PR.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
jbonofre commented on code in PR #4091: URL: https://github.com/apache/polaris/pull/4091#discussion_r3035075596 ## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ## @@ -849,14 +855,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential return responseBuilder; } -if (baseCatalog instanceof IcebergCatalog +if (!isFederated Review Comment: That's fair. +1 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091: URL: https://github.com/apache/polaris/pull/4091#discussion_r3034703540 ## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ## @@ -849,14 +855,14 @@ private LoadTableResponse.Builder buildLoadTableResponseWithDelegationCredential return responseBuilder; } -if (baseCatalog instanceof IcebergCatalog +if (!isFederated Review Comment: > If catalogFactory().createCallContextCatalog() returns something over than IcebergCatalog, the "previous" code would gracefully fall through to the else branch, I don't think that's a graceful behavior. The `else` branch is for federated catalogs, `catalogFactory().createCallContextCatalog()` return a non federated catalog, the behavior is completely wrong. After this change, the code fail fast, maybe not the best way now, but better. We can continue to refactor to make it better in followup PRs. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
jbonofre commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3032502123
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
If I may, it seems that `isFererated` flag duplicates information already
derivable from `baseCatalog`'s type.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -849,14 +855,14 @@ private LoadTableResponse.Builder
buildLoadTableResponseWithDelegationCredential
return responseBuilder;
}
-if (baseCatalog instanceof IcebergCatalog
+if (!isFederated
Review Comment:
Is it not a regression on type safety ?
If `catalogFactory().createCallContextCatalog()` returns something over than
`IcebergCatalog`, the "previous" code would gracefully fall through to the else
branch, while the new code throws a `ClassCastException` at runtime.
The `isFederated` flag and `baseCatalog`'s concrete type are set
independently - there's no compile-time guarantee they stay in sync.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3030823383
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
Like I said my preference in this situation is to do a more holistic
refactoring (specific ideas in comments above).
If you prefer a smaller change, perhaps `createCallContextCatalog()` should
return an `IcebergCatalog`? It's the only type it actually creates in runtime,
as far as I can tell.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3030823383
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
Like I said my preference in this situation is to do a more holistic
refactoring (specific ideas in comments above).
If you prefer a smaller change, perhaps `createCallContextCatalog()` should
return an `IcebergCatalog`? It's the only type is actually creates in runtime,
as far as I can tell.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3030752051
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
If `catalogFactory().createCallContextCatalog()` returns some other type,
the old code path would treat it as a federated catalog, which is incorrect.
With this change, it would fail fast instead. Which behavior do you think is
preferable?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3029861082
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
I maintain my assessment that this refactoring is not conceptually better
than the old code in terms of explicit / implicit logic (more specific comments
above).
If you prefer to emphasize `isFederated`, that's fine. However, such a
refactoring should not demote checked casts to unchecked (assumed) casts, IMHO.
Please consider what will happen if
`catalogFactory().createCallContextCatalog()` returns some other type.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3029332460
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
I think `isFederated` is exactly the semantic we want to express here. It
captures the intent directly, rather than coupling the logic to a specific
catalog class.
The goal is to distinguish behavior based on whether a catalog is federated
or not, not based on its concrete type. Tying this to `instanceof
IcebergCatalog` would mix implementation detail with business logic, which
feels less clear to me.
Do you agree that modeling this as an explicit semantic, instead of
inferring it from class type, is the right direction?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3028852129
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
My point is that the PR (according to its title) aims to make the "logic
more explicit", however, as far as I can tell the logic becomes less explicit.
Adding the `checkArgument` is an indication that a certain assumption is
made by this code. Such assumptions are not "explicit".
To be clear: I support improving this code. However, I do not see that
current PR is net beneficial. It converts one kind of assumption to another
kind of assumption, but the code is still obscure overall, IMHO :shrug:
I believe the best way to actually make this code more explicit is to
refactor it and separate federated catalogs logic and internal catalogs logic
into different classes. Other code areas will be affected, so it's not a small
effort, for sure.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3028852129
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
My point is that the PR (according to its title) aims to make the "logic
more explicit", however, I far as I can tell the logic becomes less explicit.
Adding the `checkArgument` is an indication that a certain assumption is
made by this code. Such assumptions are not "explicit".
To be clear: I support improving this code. However, I do not see that
current PR is net beneficial. It converts one kind of assumption to another
kind of assumption, but the code is still obscure overall, IMHO :shrug:
I believe the best way to actually make this code more explicit is to
refactor it and separate federated catalogs logic and internal catalogs logic
into different classes. Other code areas will be affected, so it's not a small
effort, for sure.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3025718137
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
if your concern is the implicit cast, I can make it a Precondition check
like this when not federated, would that work for you?
```
Preconditions.checkArgument(baseCatalog instanceof IcebergCatalog,
"error message");
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024789675
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
IMHO, this PR makes the logic more obscure (less explicit), so I do not see
how it fits the stated purpose (from the PR description) :shrug:
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024735393
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
I thought about that while making this refactor, but due to a lot of common
code, I decide not to do so. But I'm open to it. With the current refactor in
the this PR, it becomes easier if anyone want to do so. Is this a blocker for
this refactor?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024791186
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
Not a blocker, but I do not really see how it improves the codebase :shrug:
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024863869
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
`initializeCatalog()` decides whether a catalog is federated or not. That's
the reason we make it explicit there. A condition on catalog class type is hard
to understand, and reasoning, given there are many different catalog classes,
and all of them are Iceberg catalogs. A conditional check like `if (baseCatalog
instanceof IcebergCatalog polarisCatalog)` makes it harder to understand
whether it is a federated one or not.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024817085
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
Before this change, the code already relied on the fact that certain
catalogs are federated and others are not, but that assumption was hidden
inside a checked cast, so it wasn’t obvious when reading the flow.
With this refactor:
- The logic is now structured around the federation flag first. This was
done in the initialization method, which provide clearly distinguish whether a
catalog is federated or not.
- The branching makes it clear that we are handling two distinct cases
- The cast is still required, but it now happens in a well-defined branch,
instead of being buried in-line
So the assumption didn’t get introduced by this PR, it was already there.
What changed is that it is now surfaced explicitly in the control flow, making
the intent easier to see and reason about.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024701574
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
That assumption is in the new code - it checks the `isFedeated` flag, and
performs an unchecked (no `instanceof`) cast here. This means the code assumes
that `baseCatalog` if `isFedeated == false`, which is based purely on
`ConnectionConfigInfoDpo` and it is not clear how `ConnectionConfigInfoDpo`
affects the type of `baseCatalog`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3025007747
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
If agree that the current logic in this class is not ideal. Yet, I believe
changing _check_ casts to implicit casts is better... that's why I'm advocating
for a more holistic object-oriented approach.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
If agree that the current logic in this class is not ideal. Yet, I believe
changing _checked_ casts to implicit casts is better... that's why I'm
advocating for a more holistic object-oriented approach.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024750737
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
If needed we could introduce more guardrail to ensure the catalog of
federated catalogs and others.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024746147
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
The cast is required regardless, so I don’t see how this refactor makes
things worse. Previously, the assumption was implicit and hidden, whereas now
it’s made explicit. That’s exactly the intent of this PR.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024735393
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
I thought about that, but due to a lot of common code, I decide not to do
so. But I'm open to it. With the current refactor in the this PR, it becomes
easier if anyone want to do so. Is this a blocker for this refactor?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024676272
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
> we now implicitly assume that non-federated catalogs are instances of
IcebergCatalog
I don't understand this part. Why would we assume that now?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024701574
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
That assumption is in the new code - it checks the `isFedeated` flag, and
performs an unchecked (no `instanceof`) cast here. This means the code assumes
that `baseCatalog` is of type `IcebergCatalog` if `isFedeated == false`, which
is based purely on `ConnectionConfigInfoDpo` and it is not clear how
`ConnectionConfigInfoDpo` affects the type of `baseCatalog`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3025002741
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
`initializeCatalog()` decides the value of the `isFederated` fields. It does
not decide the type of `baseCatalog`.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
`initializeCatalog()` decides the value of the `isFederated` field. It does
not decide the type of `baseCatalog`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024670225
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
Can you be more specific about more object-oriented design?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024612334
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
I would not call this "more explicit logic". IMHO, the logic becomes less
explicit because instead of using a checked cast (line 239 before) we now
implicitly assume that non-federated catalogs are instances of `IcebergCatalog`
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
I'd prefer a different kind of refactoring for this code so as to remove
these if/else constructs and have a more object-oriented design.
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -231,23 +235,6 @@ private boolean shouldDecodeToken() {
return realmConfig().getConfig(LIST_PAGINATION_ENABLED,
getResolvedCatalogEntity());
}
- public ListNamespacesResponse listNamespaces(
Review Comment:
nit: moving code is fine, but it's outside the declared scope of this PR
(per PR description) and makes the "real diff" harder to deduce 🤷
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3025007747
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
If agree that the current logic in this class is not ideal. Yet, I believe
changing _checked_ casts to implicit casts is _not_ better... that's why I'm
advocating for a more holistic object-oriented approach.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024829567
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+} else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
IMHO a _checked_ cast is more explicit and easier to reason about than an
`if` on a flag that is derived from the config, which is not directly related
to the type of the variable being cast.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024688085
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
-if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+if (isFederated) {
Review Comment:
Avoid if/else blocks on the "federated" flag but use different sub-classes
to implement local and federated catalogs.
The diff will be larger, of course, but I hope the overall code quality will
be better.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024667271
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -231,23 +235,6 @@ private boolean shouldDecodeToken() {
return realmConfig().getConfig(LIST_PAGINATION_ENABLED,
getResolvedCatalogEntity());
}
- public ListNamespacesResponse listNamespaces(
Review Comment:
What's the suggested action item?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on PR #4091: URL: https://github.com/apache/polaris/pull/4091#issuecomment-4165322688 the test failure seems unrelated. ``` > ERROR: failed to build: failed to solve: registry.access.redhat.com/ubi9/openjdk-21-runtime:1.24-2.1774011801: failed to resolve source metadata for registry.access.redhat.com/ubi9/openjdk-21-runtime:1.24-2.1774011801: failed to read expected number of byt at io.smallrye.common.process.PipelineRunner.collectProblems(PipelineRunner.java:559) at io.smallrye.common.process.ProcessRunner.complete(ProcessRunner.java:165) at io.smallrye.common.process.ProcessRunner.run(ProcessRunner.java:95) at io.smallrye.common.process.ProcessBuilderImpl.run(ProcessBuilderImpl.java:199) at io.smallrye.common.process.ProcessBuilderImpl$ViewImpl.run(ProcessBuilderImpl.java:268) at io.quarkus.container.image.docker.common.deployment.CommonProcessor.buildImage(CommonProcessor.java:337) at io.quarkus.container.image.docker.deployment.DockerProcessor.createContainerImage(DockerProcessor.java:132) at io.quarkus.container.image.docker.deployment.DockerProcessor.createContainerImage(DockerProcessor.java:38) at io.quarkus.container.image.docker.common.deployment.CommonProcessor.buildFromJar(CommonProcessor.java:95) at io.quarkus.container.image.docker.deployment.DockerProcessor.dockerBuildFromJar(DockerProcessor.java:69) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:898) at io.quarkus.builder.BuildContext.run(BuildContext.java:242) at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18) at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2651) at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2630) at org.jboss.threads.EnhancedQueueExecutor.runThreadBody(EnhancedQueueExecutor.java:1622) at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1589) at java.base/java.lang.Thread.run(Thread.java:1583) at org.jboss.threads.JBossThread.run(JBossThread.java:501) ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain closed pull request #4091: Service: Make federation logic more explicit URL: https://github.com/apache/polaris/pull/4091 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3017266734
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -218,6 +217,11 @@ public abstract class IcebergCatalogHandler extends
CatalogHandler implements Au
@SuppressWarnings("immutables:incompat")
private ViewCatalog viewCatalog = null;
+ // Indicates whether the catalog is a non-federated (Polaris-managed)
catalog.
+ // Non-federated catalogs support additional capabilities such as pagination,
+ // location transformation, credential vending, and multi-table transactions.
+ private boolean isNonFederated = false;
Review Comment:
Good idea, implemented option 2.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
singhpk234 commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3014046784
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -218,6 +217,11 @@ public abstract class IcebergCatalogHandler extends
CatalogHandler implements Au
@SuppressWarnings("immutables:incompat")
private ViewCatalog viewCatalog = null;
+ // Indicates whether the catalog is a non-federated (Polaris-managed)
catalog.
+ // Non-federated catalogs support additional capabilities such as pagination,
+ // location transformation, credential vending, and multi-table transactions.
+ private boolean isNonFederated = false;
Review Comment:
Option 1 : how about `isManaged` ? which means catalogs managed by Polaris,
this can replace nonFederated
Option 2: also i wonder what if we filp the neg and positive cases
if (isFederated) {
// existing
} else {
// existing
}
wdyt ? isNon went hand in was a bit hard to reason, though i don't strongly
feel about it, let me know what do you think or option #1 or option #2
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
singhpk234 commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3014046784
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -218,6 +217,11 @@ public abstract class IcebergCatalogHandler extends
CatalogHandler implements Au
@SuppressWarnings("immutables:incompat")
private ViewCatalog viewCatalog = null;
+ // Indicates whether the catalog is a non-federated (Polaris-managed)
catalog.
+ // Non-federated catalogs support additional capabilities such as pagination,
+ // location transformation, credential vending, and multi-table transactions.
+ private boolean isNonFederated = false;
Review Comment:
Option 1 : how about `isManaged` ? which means catalogs managed by Polaris
Option 2: also i wonder what if we filp the neg and positive cases
if (isFederated) {
// existing
} else {
// existing
}
wdyt ? isNon went hand in was a bit hard to reason, though i don't strongly
feel about it, let me know what do you think or option #1 or option #2
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3013763802
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -218,6 +217,11 @@ public abstract class IcebergCatalogHandler extends
CatalogHandler implements Au
@SuppressWarnings("immutables:incompat")
private ViewCatalog viewCatalog = null;
+ // Indicates whether the catalog is a non-federated (Polaris-managed)
catalog.
+ // Non-federated catalogs support additional capabilities such as pagination,
+ // location transformation, credential vending, and multi-table transactions.
+ private boolean isNonFederated = false;
Review Comment:
`isFederated` was one of my versions. I changed to `isNonFederated` as there
are many `!isFederated`. I can change it back if you feel strongly about it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
singhpk234 commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3013545310
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -218,6 +217,11 @@ public abstract class IcebergCatalogHandler extends
CatalogHandler implements Au
@SuppressWarnings("immutables:incompat")
private ViewCatalog viewCatalog = null;
+ // Indicates whether the catalog is a non-federated (Polaris-managed)
catalog.
+ // Non-federated catalogs support additional capabilities such as pagination,
+ // location transformation, credential vending, and multi-table transactions.
+ private boolean isNonFederated = false;
Review Comment:
How about we have `isFederated` a flag ?
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -218,6 +217,11 @@ public abstract class IcebergCatalogHandler extends
CatalogHandler implements Au
@SuppressWarnings("immutables:incompat")
private ViewCatalog viewCatalog = null;
+ // Indicates whether the catalog is a non-federated (Polaris-managed)
catalog.
+ // Non-federated catalogs support additional capabilities such as pagination,
+ // location transformation, credential vending, and multi-table transactions.
+ private boolean isNonFederated = false;
Review Comment:
or isManaged ? to have same semantics
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Service: Make federation logic more explicit [polaris]
flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3013451463
##
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##
@@ -231,23 +235,6 @@ private boolean shouldDecodeToken() {
return realmConfig().getConfig(LIST_PAGINATION_ENABLED,
getResolvedCatalogEntity());
}
- public ListNamespacesResponse listNamespaces(
Review Comment:
Move it down to group related methods together
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
