bharathv commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r543981842
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -454,37 +453,39 @@ public static ReplicationPeerConfig
appendTableCFsToReplicationPeerConfig(
}
/**
- * Helper method to add base peer configs from Configuration to
ReplicationPeerConfig
- * if not present in latter.
+ * Helper method to add/removev base peer configs from Configuration to
ReplicationPeerConfig
*
* 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".
Original value
- * of conf is retained if already present in ReplicationPeerConfig.
+ * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1;k3=""".
+ * If value is empty, it will remove the existing key-value from peer config.
*
* @param conf Configuration
* @return ReplicationPeerConfig containing updated configs.
*/
- public static ReplicationPeerConfig
addBasePeerConfigsIfNotPresent(Configuration conf,
+ public static ReplicationPeerConfig
updateReplicationBasePeerConfigs(Configuration conf,
ReplicationPeerConfig receivedPeerConfig) {
- String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, "");
ReplicationPeerConfigBuilder copiedPeerConfigBuilder =
ReplicationPeerConfig.
newBuilder(receivedPeerConfig);
- Map<String,String> receivedPeerConfigMap =
receivedPeerConfig.getConfiguration();
+ Map<String, String> receivedPeerConfigMap =
receivedPeerConfig.getConfiguration();
+ String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, "");
if (basePeerConfigs.length() != 0) {
Map<String, String> basePeerConfigMap =
Splitter.on(';').trimResults().omitEmptyStrings()
.withKeyValueSeparator("=").split(basePeerConfigs);
- for (Map.Entry<String,String> entry : basePeerConfigMap.entrySet()) {
+ for (Map.Entry<String, String> entry : basePeerConfigMap.entrySet()) {
String configName = entry.getKey();
String configValue = entry.getValue();
- // Only override if base config does not exist in existing peer configs
- if (!receivedPeerConfigMap.containsKey(configName)) {
+ if (Strings.isNullOrEmpty(configValue)) {
+ copiedPeerConfigBuilder.removeConfiguration(configName);
Review comment:
Perhaps add a quick comment on why we do this and possible edge cases?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -200,8 +198,8 @@ public void testHFileCyclicReplication() throws Exception {
// Load 100 rows for each hfile range in cluster '0' and validate
whether its been replicated
// to cluster '1'.
byte[][][] hfileRanges =
- new byte[][][] { new byte[][] { Bytes.toBytes("aaaa"),
Bytes.toBytes("cccc") },
- new byte[][] { Bytes.toBytes("ddd"), Bytes.toBytes("fff") }, };
+ new byte[][][] { new byte[][] { Bytes.toBytes("aaaa"),
Bytes.toBytes("cccc") },
Review comment:
All these indent changes intentional? (painful for backporting)
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -502,25 +500,81 @@ public void testBasePeerConfigsForPeerMutations()
utilities[0].restartHBaseCluster(1);
admin = utilities[0].getAdmin();
- // Both retains the value of base configuration 1 value as before
restart.
- // Peer 1 (Update value), Peer 2 (Base Value)
- Assert.assertEquals(firstCustomPeerConfigUpdatedValue,
admin.getReplicationPeerConfig("1").
+ // Configurations should be updated after restart again
+ Assert.assertEquals(firstCustomPeerConfigValue,
admin.getReplicationPeerConfig("1").
getConfiguration().get(firstCustomPeerConfigKey));
Assert.assertEquals(firstCustomPeerConfigValue,
admin.getReplicationPeerConfig("2").
getConfiguration().get(firstCustomPeerConfigKey));
- // Peer 1 gets new base config as part of restart.
Assert.assertEquals(secondCustomPeerConfigValue,
admin.getReplicationPeerConfig("1").
getConfiguration().get(secondCustomPeerConfigKey));
- // Peer 2 retains the updated value as before restart.
- Assert.assertEquals(secondCustomPeerConfigUpdatedValue,
admin.getReplicationPeerConfig("2").
+ Assert.assertEquals(secondCustomPeerConfigValue,
admin.getReplicationPeerConfig("2").
getConfiguration().get(secondCustomPeerConfigKey));
} finally {
shutDownMiniClusters();
baseConfiguration.unset(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG);
}
}
+ @Test
+ public void testBasePeerConfigsRemovalForPeerMutations()
+ throws Exception {
+ LOG.info("testBasePeerConfigsForPeerMutations");
+ String firstCustomPeerConfigKey = "hbase.xxx.custom_config";
+ String firstCustomPeerConfigValue = "test";
+
+ try {
+
baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+
firstCustomPeerConfigKey.concat("=").concat(firstCustomPeerConfigValue));
+ startMiniClusters(2);
+ addPeer("1", 0, 1);
+ Admin admin = utilities[0].getAdmin();
+
+ // Validates base configs 1 is present for both peer.
+ Assert.assertEquals(firstCustomPeerConfigValue,
admin.getReplicationPeerConfig("1").
+ getConfiguration().get(firstCustomPeerConfigKey));
+
+ utilities[0].getConfiguration().unset(ReplicationPeerConfigUtil.
+ HBASE_REPLICATION_PEER_BASE_CONFIG);
+ utilities[0].getConfiguration().set(ReplicationPeerConfigUtil.
+ HBASE_REPLICATION_PEER_BASE_CONFIG,
firstCustomPeerConfigKey.concat("=").concat(""));
+
+
+ utilities[0].shutdownMiniHBaseCluster();
+ utilities[0].restartHBaseCluster(1);
+ admin = utilities[0].getAdmin();
+
+ // Configurations should be removed after restart again
+ Assert.assertNull(admin.getReplicationPeerConfig("1")
+ .getConfiguration().get(firstCustomPeerConfigKey));
+ } finally {
+ shutDownMiniClusters();
+
baseConfiguration.unset(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG);
+ }
+ }
+
+ @Test
+ public void testRemoveBasePeerConfigWithoutExistingConfigForPeerMutations()
Review comment:
same nit as above.
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -502,25 +500,81 @@ public void testBasePeerConfigsForPeerMutations()
utilities[0].restartHBaseCluster(1);
admin = utilities[0].getAdmin();
- // Both retains the value of base configuration 1 value as before
restart.
- // Peer 1 (Update value), Peer 2 (Base Value)
- Assert.assertEquals(firstCustomPeerConfigUpdatedValue,
admin.getReplicationPeerConfig("1").
+ // Configurations should be updated after restart again
+ Assert.assertEquals(firstCustomPeerConfigValue,
admin.getReplicationPeerConfig("1").
getConfiguration().get(firstCustomPeerConfigKey));
Assert.assertEquals(firstCustomPeerConfigValue,
admin.getReplicationPeerConfig("2").
getConfiguration().get(firstCustomPeerConfigKey));
- // Peer 1 gets new base config as part of restart.
Assert.assertEquals(secondCustomPeerConfigValue,
admin.getReplicationPeerConfig("1").
getConfiguration().get(secondCustomPeerConfigKey));
- // Peer 2 retains the updated value as before restart.
- Assert.assertEquals(secondCustomPeerConfigUpdatedValue,
admin.getReplicationPeerConfig("2").
+ Assert.assertEquals(secondCustomPeerConfigValue,
admin.getReplicationPeerConfig("2").
getConfiguration().get(secondCustomPeerConfigKey));
} finally {
shutDownMiniClusters();
baseConfiguration.unset(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG);
}
}
+ @Test
+ public void testBasePeerConfigsRemovalForPeerMutations()
Review comment:
nit: Use a word other than mutation given it has a different meaning in
HBase context? or may be just call it testBasePeerConfigRemoval..
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -743,11 +797,11 @@ private void rollWALAndWait(final HBaseTestingUtility
utility, final TableName t
// listen for successful log rolls
final WALActionsListener listener = new WALActionsListener() {
- @Override
- public void postLogRoll(final Path oldPath, final Path newPath)
throws IOException {
- latch.countDown();
- }
- };
+ @Override
Review comment:
same as above, indent changes intentional? If not undo them?
----------------------------------------------------------------
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:
us...@infra.apache.org
