bharathv commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r473434176
##########
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();
}
+ /**
Review comment:
nit: Sample configuration block not needed. Its kind of obvious. Instead
add the configuration value formatting to the javadoc of the config? (and also
what the default is).
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -60,6 +60,7 @@
public final class ReplicationPeerConfigUtil {
private static final Logger LOG =
LoggerFactory.getLogger(ReplicationPeerConfigUtil.class);
+ public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG=
"hbase.replication.peer.default.config";
Review comment:
Make it private?
##########
File path:
hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,52 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ 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 testDefaultReplicationPeerConfigOverrideIfAlreadySet(){
Review comment:
I think we need more coverage for the following cases.
- Existing peer config gets the config override
- Admin code paths (for getPeerConfig, updatePeerConfig, etc) work well with
the overlays (updating an existing / non-existing config etc)
##########
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);
Review comment:
nit: get(CONFIG, default)
##########
File path:
hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeers.java
##########
@@ -144,7 +145,12 @@ private ReplicationPeerImpl createPeer(String peerId)
throws ReplicationExceptio
SyncReplicationState syncReplicationState =
peerStorage.getPeerSyncReplicationState(peerId);
SyncReplicationState newSyncReplicationState =
peerStorage.getPeerNewSyncReplicationState(peerId);
+
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(this.conf, peerConfig);
+ peerStorage.updatePeerConfig(peerId,updatedPeerConfig);
Review comment:
I was thinking this happens only in the master code paths. Ex:
ReplicationPeerManager#create (for existing peers) or addPeer() for new peers
etc. That way the configuration in storage remains consistent.
Doing from the RS code paths (ReplicationPeers) means that if different RS
run with different configs it can result in a different final state (depending
which RS does this RPC last). Also doing this from HMaster seems logical since
this is more like an admin operation whereas RS based codepaths are just
consumers of this config.
##########
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){
Review comment:
Mind fixing the check-style issues from precommit? Bunch of overflows.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,50 @@ public static ReplicationPeerConfig
appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.base.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;
+ hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add base peer configs from HBase Configuration to
ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
Review comment:
nit: return value javadoc seems wrong.
##########
File path:
hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,52 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
Review comment:
Two tests can be merged into one.
##########
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:
Also Splitter from Guava provides a clean API to do this parsing (couple
of lines of code).
----------------------------------------------------------------
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]
