poojanilangekar commented on code in PR #1925:
URL: https://github.com/apache/polaris/pull/1925#discussion_r2167615225
##########
polaris-core/src/main/java/org/apache/polaris/core/connection/AuthenticationType.java:
##########
@@ -33,6 +33,7 @@ public enum AuthenticationType {
NULL_TYPE(0),
OAUTH(1),
BEARER(2),
+ NO_AUTH(3),
Review Comment:
While `NULL_TYPE` is not explicitly used in the codebase, it is implied (and
therefore used) in the default case of switch statements. I can go over a more
explicit case for `ConnectionType` here:
1. Assume that sometime in the future, Polaris adds support for `HIVE`
connections.
2. The user then creates an external catalog called `hive-catalog` of type
`HIVE`. At which point, Polaris will store the corresponding
ConnectionConfigInfoDpo into the underlying persistence layer. While creating
the persistence object, the constructor similar to
`IcebergRestConnectionConfigInfoDpo`, called `HiveConnectionConfigInfoDpo` will
create this object with enums defined internally by the Polaris server. It'd
make a call of the form `ConnectionType.HIVE.getCode()`.
3. Further, let's assume that one of the two happens (1) we rollback the
hive change the newer versions of Polaris don't support Hive connections, or
(2) the Polaris instance is rebooted with an older version. In both cases, the
Polaris instance won't recognize the `HIVE` connection type.
4. Lastly, let's say the user wants to perform some operation on the
`hive-catalog`. The `IcebergCatalogHandler::initializeCatalog()` would now read
the persisted object and parse it's information, specifically, let's look at
this snippet:
``` ConnectionType connectionType =
ConnectionType.fromCode(connectionConfigInfoDpo.getConnectionTypeCode());
switch (connectionType) {
case ICEBERG_REST:
SessionCatalog.SessionContext context =
SessionCatalog.SessionContext.createEmpty();
federatedCatalog =
new RESTCatalog(
context,
(config) ->
HTTPClient.builder(config)
.uri(config.get(org.apache.iceberg.CatalogProperties.URI))
.build());
federatedCatalog.initialize(
((IcebergRestConnectionConfigInfoDpo)
connectionConfigInfoDpo).getRemoteCatalogName(),
connectionConfigInfoDpo.asIcebergCatalogProperties(getUserSecretsManager()));
break;
case HADOOP:
federatedCatalog = new HadoopCatalog();
federatedCatalog.initialize(
((HadoopConnectionConfigInfoDpo)
connectionConfigInfoDpo).getWarehouse(),
connectionConfigInfoDpo.asIcebergCatalogProperties(getUserSecretsManager()));
break;
default:
throw new UnsupportedOperationException(
"Connection type not supported: " + connectionType);
}
```
Since `HIVE` is not a valid connection type in this Polaris instance, the
`ConnectionType::fromCode()` function will return `NULL_TYPE` and hence
activate the default case.
This was my understanding of why we need a `NULL_TYPE`. Essentially, since
we always want to support not just backward compatibility but also forward
compatibility wherein the data store might contain some objects/types that the
current Polaris instance does not recognize. The `NULL_TYPE` provide a nice
catch-all type to allow appropriate error handling.
--
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]
