bharathv commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r478608776
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -441,6 +442,41 @@ public void testCyclicReplication3() throws Exception {
}
}
+ /**
+ * Tests that base replication peer configs are applied on peer creation
+ * and the is overriden if updated as part of updateReplicationPeerConfig()
+ *
+ */
+ @Test
+ public void testBasePeerConfigsAppliedOnPeerCreationAndUpdate()
+ throws Exception {
+ LOG.info("testBasePeerConfigsAppliedOnPeerCreation");
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+ try {
+
baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+ startMiniClusters(2);
+ addPeer("1", 0, 1);
+ Admin admin = utilities[0].getAdmin();
+
+ Assert.assertEquals(customPeerConfigValue,
admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig =
ReplicationPeerConfig.
+ newBuilder(admin.getReplicationPeerConfig("1")).
+
putConfiguration(customPeerConfigKey,customPeerConfigUpdatedValue).build();
+ admin.updateReplicationPeerConfig("1", updatedReplicationPeerConfig);
+
+ Assert.assertEquals(customPeerConfigUpdatedValue,
admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+ }finally {
Review comment:
nit: } finally
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig
appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * Helper method to add base peer configs from Configuration to
ReplicationPeerConfig
+ * if not present in latter.
+ *
+ * This merges the user supplied peer configuration
+ * {@link org.apache.hadoop.hbase.replication.ReplicationPeerConfig} with
peer configs
+ * provided as property hbase.replication.peer.base.configs in hbase
configuration.
+ * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else
null.
+ */
+ public static ReplicationPeerConfig
addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs =
conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder =
ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> peerConfigurations =
receivedPeerConfig.getConfiguration();
+
+ if (defaultPeerConfigs != null && defaultPeerConfigs.length() != 0) {
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
+
+ for (String defaultPeerConfig : defaultPeerConfigList) {
+ String[] configSplit = defaultPeerConfig.split("=");
+
+ if (configSplit != null && configSplit.length == 2) {
+ String configName = configSplit[0];
+ String configValue = configSplit[1];
+
+ // Only override if base config does not exist in existing peer
configs
+ if (!peerConfigurations.containsKey(configName)) {
+ copiedPeerConfigBuilder.putConfiguration(configName,configValue);
+ isPeerConfigChanged = true;
+ }
+ }
+ }
+ }
+
+ return isPeerConfigChanged ? copiedPeerConfigBuilder.build() : null;
Review comment:
Why this? Just change the receivedPeerConfig in place?
##########
File path:
hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +219,51 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testBaseReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+
+ String customPeerConfigSecondKey = "hbase.xxx.custom_second_config";
+ String customPeerConfigSecondValue = "testSecond";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+ // custom config not present
+
assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey),
null);
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+
customPeerConfigKey.concat("=").concat(customPeerConfigValue).concat(";").
+
concat(customPeerConfigSecondKey).concat("=").concat(customPeerConfigSecondValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig =
ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
+
+ assertEquals(customPeerConfigValue,
updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigKey));
+ assertEquals(customPeerConfigSecondValue,
updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigSecondKey));
+ }
+
+ @Test
+ public void testBaseReplicationPeerConfigDoesNotOverrideIfAlreadySet(){
+
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigOldValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+
+ ReplicationPeerConfig existingReplicationPeerConfig =
ReplicationPeerConfig.
+ newBuilder(getConfig(1))
+ .putConfiguration(customPeerConfigKey,customPeerConfigOldValue).build();
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigUpdatedValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig =
ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
Review comment:
I lost your comment in the force push, but I think we can merge both
these tests into testReplicationBaseConfig() or some such... We cam just add
these last two lines in the above test and we don't need all the other
boilerplate.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig
appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+
<value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to
ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig
addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig
receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder =
ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs =
conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
+
+ Map<String,String> peerConfigurations =
receivedPeerConfig.getConfiguration();
+
+ if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
Review comment:
Looks like you missed this, this code can be condensed into
Splitter.on(';').withKeyValueSeparator('=').split(value).trimResults();
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig
appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * Helper method to add base peer configs from Configuration to
ReplicationPeerConfig
+ * if not present in latter.
+ *
+ * This merges the user supplied peer configuration
+ * {@link org.apache.hadoop.hbase.replication.ReplicationPeerConfig} with
peer configs
+ * provided as property hbase.replication.peer.base.configs in hbase
configuration.
+ * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else
null.
+ */
+ public static ReplicationPeerConfig
addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
Review comment:
nit: space between ) {
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig
appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * Helper method to add base peer configs from Configuration to
ReplicationPeerConfig
+ * if not present in latter.
+ *
+ * This merges the user supplied peer configuration
+ * {@link org.apache.hadoop.hbase.replication.ReplicationPeerConfig} with
peer configs
+ * provided as property hbase.replication.peer.base.configs in hbase
configuration.
+ * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else
null.
+ */
+ public static ReplicationPeerConfig
addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs =
conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
Review comment:
nit: space between , null
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -441,6 +442,41 @@ public void testCyclicReplication3() throws Exception {
}
}
+ /**
+ * Tests that base replication peer configs are applied on peer creation
+ * and the is overriden if updated as part of updateReplicationPeerConfig()
+ *
+ */
+ @Test
+ public void testBasePeerConfigsAppliedOnPeerCreationAndUpdate()
+ throws Exception {
+ LOG.info("testBasePeerConfigsAppliedOnPeerCreation");
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+ try {
+
baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+ startMiniClusters(2);
+ addPeer("1", 0, 1);
+ Admin admin = utilities[0].getAdmin();
+
+ Assert.assertEquals(customPeerConfigValue,
admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig =
ReplicationPeerConfig.
+ newBuilder(admin.getReplicationPeerConfig("1")).
+
putConfiguration(customPeerConfigKey,customPeerConfigUpdatedValue).build();
+ admin.updateReplicationPeerConfig("1", updatedReplicationPeerConfig);
Review comment:
Can you also add some tests for deleting a configuration? delete an
override, base config should be picked up if it is present
Also, add another peer and make sure it only has the base configuration and
not the updated values above.
Also, add a peer, update base configuration, restart cluster, new base
should be picked up (our target usecase essentially).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
