-
Notifications
You must be signed in to change notification settings - Fork 332
Modularize federation (Option 2) #2332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
If @snazy is okay with using this approach for HMS federation, then I think it's preferable to option 1 |
snazy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, thanks for removing the conditional build parameter. One comment though. But otherwise I'm fine with it.
| public static final String ICEBERG_REST_FACTORY_IDENTIFIER = "ICEBERG_REST"; | ||
| public static final String HADOOP_FACTORY_IDENTIFIER = "HADOOP"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THose are superfluous, as those are the enum names, and can therefore be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need these because we can't use ConnnectionType.ICEBERG_REST.name() or ConnectionType.HADOOP.name(). The annotations needs to be constant expressions. Here's the error for your reference.
The value for annotation attribute Identifier.value must be a constant expressionJava(536871525)
| public String getFactoryIdentifier() { | ||
| switch (this) { | ||
| case ICEBERG_REST: | ||
| return ICEBERG_REST_FACTORY_IDENTIFIER; | ||
| case HADOOP: | ||
| return HADOOP_FACTORY_IDENTIFIER; | ||
| default: | ||
| throw new UnsupportedOperationException( | ||
| "No factory identifier for connection type: " + this); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to just return name().toLowerCase(Locale.ROOT) – since most CDI identifiers in Polaris (if not all) are lower-case.
| public String getFactoryIdentifier() { | |
| switch (this) { | |
| case ICEBERG_REST: | |
| return ICEBERG_REST_FACTORY_IDENTIFIER; | |
| case HADOOP: | |
| return HADOOP_FACTORY_IDENTIFIER; | |
| default: | |
| throw new UnsupportedOperationException( | |
| "No factory identifier for connection type: " + this); | |
| } | |
| } | |
| public String getFactoryIdentifier() { | |
| return name().toLowerCase(Locale.ROOT); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use the name().toLowerCase() directly because of two reasons:
- We need the annotations to be constants. (Please see my comment above)
- We need to explicitly handle the NULL_TYPE/default case.
To make things consistent, I've changed the declarations to use lower case.
dennishuo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Looks like other comments have also been addressed, merging now. |
Based on the discussion in the community sync, this PR modularizes federation calls to a separate factory. The specific instances of the NonRESTCatalogFactory is loaded only when a non-REST catalog is initialized and accessed. Beyond the catalog initialization, all the calls are identical for internal and federated catalogs.
Based on the discussion in #2301, this PR removes the compile-time flag used to build the
HadoopFederatedCatalogFactory.Testing:
Applied the regtest in PR #2286.