[GitHub] [hbase] bharathv commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org