-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add personalized configuration parameters for each metastore. #315
Conversation
cat waggle-dance-server.yml verbose: true |
@@ -45,7 +43,21 @@ public class CloseableThriftHiveMetastoreIfaceClientFactory { | |||
public CloseableThriftHiveMetastoreIface newInstance(AbstractMetaStore metaStore) { | |||
Map<String, String> properties = new HashMap<>(); | |||
if (waggleDanceConfiguration.getConfigurationProperties() != null) { | |||
properties.putAll(waggleDanceConfiguration.getConfigurationProperties()); | |||
// properties.putAll(waggleDanceConfiguration.getConfigurationProperties()); | |||
Map<String, String> serverConfigMap=waggleDanceConfiguration.getConfigurationProperties(); |
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.
I suggest we move these properties to the AbstractMetastore class and make it part of the waggle-dance-federation.yml and this code can become much simpler like:
properties.putAll(waggleDanceConfiguration.getConfigurationProperties());
//override per metastore
properties.putAll(metastore.getConfigurationProperties());
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.
Thank you please have a look and my comment.
primary-meta-store: |
@@ -47,6 +47,8 @@ public CloseableThriftHiveMetastoreIface newInstance(AbstractMetaStore metaStore | |||
if (waggleDanceConfiguration.getConfigurationProperties() != null) { | |||
properties.putAll(waggleDanceConfiguration.getConfigurationProperties()); | |||
} | |||
//override per metastore | |||
properties.putAll(metaStore.getConfigurationProperties()); |
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.
Could you add a junit test please?
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.
added
private @NotBlank String name; | ||
private @NotBlank String remoteMetaStoreUris; | ||
private @Valid MetastoreTunnel metastoreTunnel; | ||
private @NotNull AccessControlType accessControlType = AccessControlType.READ_ONLY; | ||
private transient @JsonProperty @NotNull MetaStoreStatus status = MetaStoreStatus.UNKNOWN; | ||
private long latency = 0; | ||
private transient @JsonIgnore HashBiMap<String, String> databaseNameBiMapping = HashBiMap.create(); | ||
private Map<String, String> configurationProperties = Collections.emptyMap(); | ||
private Map<String, String> configurationProperties;// = Collections.emptyMap(); |
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.
isn't it nicer to instantiate as emptyMap so you don't need the null check?
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.
@yangyuxia if you could revert or explain why this change was made I'm happy to approver once that's done,thanks!
@@ -51,15 +51,15 @@ public abstract class AbstractMetaStore { | |||
private List<String> writableDatabaseWhitelist; | |||
private List<String> mappedDatabases; | |||
private @Valid List<MappedTables> mappedTables; | |||
private Map<String, String> databaseNameMapping = Collections.emptyMap(); | |||
private transient Map<String, String> databaseNameMapping = Collections.emptyMap(); |
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.
why this had to change?
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.
@yangyuxia if you could revert or explain why this change was made I'm happy to approver once that's done,thanks!
@@ -243,4 +244,12 @@ public String toString() { | |||
.toString(); | |||
} | |||
|
|||
public Map<String, String> getConfigurationProperties() { |
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.
Please move these two methods below the setDatabaseNameMapping
method to keep them in sync with the property definitions. Normally, toString
(if defined) is the last method in a class.
@@ -47,6 +47,10 @@ public CloseableThriftHiveMetastoreIface newInstance(AbstractMetaStore metaStore | |||
if (waggleDanceConfiguration.getConfigurationProperties() != null) { | |||
properties.putAll(waggleDanceConfiguration.getConfigurationProperties()); | |||
} | |||
//override per metastore |
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.
The comment isn't needed.
nit: there's a space missing between if
and (
. You might also want to leave an empty line before the return
statement to improve readability.
|
||
factory.newInstance(newFederatedInstance("fed1", THRIFT_URI)); | ||
FederatedMetaStore fed1 = newFederatedInstance("fed1", THRIFT_URI); | ||
fed1.setConfigurationProperties(Collections.singletonMap(ConfVars.METASTORE_KERBEROS_PRINCIPAL.varname,"hive/[email protected]")); |
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.
Nit: there's a space missing between the key and the value.
Thanks for the contribution. Please update the readme explaining how this works and how it can be utilised. Thanks. |
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.
Thanks!
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.
Thank you
using new HashMap so the generated Yaml doesn't generate an anchor (reference &id001)
fixing test
…aGroup#315) * Add personalized configuration parameters for each metastore. * Add personalized configuration parameters for each metastore * Recover * Update junit test * Update Junit Test * Update Junit Test * Update Junit Test * Format the code and update the readme * Revert * Update FederatedMetaStoreTest.java * Update PrimaryMetaStoreTest.java * Update AbstractMetaStore.java using new HashMap so the generated Yaml doesn't generate an anchor (reference &id001) * Update YamlFederatedMetaStoreStorageTest.java fixing test --------- Co-authored-by: yangyx <[email protected]> Co-authored-by: Patrick Duin <[email protected]>
📝 Description
Different hive Metastores may have different parameter values for the same parameter. If multiple federated Metastores are configured in waggle dance, you can configure their own parameter values for each metastore.
For example, when hivemetastore enabled the kerberos authentication, the value of the "hive.metastore.kerberos.principal", some metastore is configured to "hive/_HOST@Realm", but some metastore is configured as "hive/{clusterName}@realm", so you need to configure personalized configuration parameters for each metastore.
🔗 Related Issues