[GitHub] [hbase] saintstack 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>]

saintstack commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r474300228



##########
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:
       Is this 'default' or 'base' configuration for all peers?

##########
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){

Review comment:
       See how rest of code has space between keyword and brackets as in 'if (' rather than 'if('.... you do this a few times in this PR. 

##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,40 @@ 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";
+
+    ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+    // custom config not present
+    assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);
+
+    Configuration conf = UTIL.getConfiguration();
+    conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_DEFAULT_CONFIG,
+      customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+
+    ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);

Review comment:
       Long lines?  100chars is max.

##########
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:
       ';' is safe character to use as delimiter for sure? Will never be part of a config value? Are the constraint on peer values at all so you could choose a delimiter that was outside of the constraint set?




----------------------------------------------------------------
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