You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/12/14 17:42:02 UTC

[GitHub] [hbase] sandeepvinayak opened a new pull request #2778: HBASE-25383: Ability to update and remove peer base config

sandeepvinayak opened a new pull request #2778:
URL: https://github.com/apache/hbase/pull/2778


   Currently, we cannot update the peer based config even if we change the value of config.
   Secondly, once the configuration is added, there is not smooth way to remove the peer config. 
   This patch adds an ability to update and remove the peer config if required.  


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-744630875


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 33s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 22s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 48s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   1m 42s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   3m 32s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 30s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   0m 27s |  The patch passed checkstyle in hbase-client  |
   | +1 :green_heart: |  checkstyle  |   0m 13s |  hbase-replication: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)  |
   | +1 :green_heart: |  checkstyle  |   1m  4s |  The patch passed checkstyle in hbase-server  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  hadoopcheck  |  17m 32s |  Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.  |
   | +1 :green_heart: |  spotbugs  |   4m  3s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 37s |  The patch does not generate ASF License warnings.  |
   |  |   |  45m 30s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux 4daf166a2948 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Max. process+thread count | 94 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/console |
   | versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] sandeepvinayak commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
sandeepvinayak commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r542772730



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -455,36 +458,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
 
   /**
    * 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". Original value
-   * of conf is retained if already present in ReplicationPeerConfig.
+   * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1".
    *
    * @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, "");
+    String removeBasePeerConfigs = conf.get(HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG, "");
     ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
       newBuilder(receivedPeerConfig);
-    Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
 
+    // remove the peer configurations specified in the conf
+    if (removeBasePeerConfigs.length() != 0) {
+      List<String> removeBasePeerConfigList = Splitter.on(';').trimResults()
+        .omitEmptyStrings().splitToList(removeBasePeerConfigs);
+      for (String peerConfigToRemove : removeBasePeerConfigList) {
+        copiedPeerConfigBuilder.removeConfiguration(peerConfigToRemove);
+      }
+    }
+
+    Map<String, String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
+    String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, "");

Review comment:
       @gjacoby126 That is correct. I will add a log here. 




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



[GitHub] [hbase] virajjasani edited a comment on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-746356621


   I think this approach of removing key if `{"key"=""}` is provided as updated peer base config, sounds good one.
   
   > @virajjasani Do you think if it is standard to have conf upgrade before the HBase rolling upgrade? If not, we might need to rework on updating the peer configs
   
   For rolling upgrade, updating configs in new bits is common before performing rolling restart of master and RS (which can also update config symlinks e.g /etc/conf/hbase), hence this patch should fit well for upgrades IMHO. With this patch, it's just like updating any other config.
   
   +1 for the PR overall once @bharathv and @gjacoby126 's remaining comments are addressed.


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-744710565


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 12s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 22s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 45s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 41s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 34s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 18s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 15s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 35s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 42s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 42s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m 49s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 16s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  3s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 37s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 156m 11s |  hbase-server in the patch passed.  |
   |  |   | 188m 41s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 9a147988476d 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Default Java | AdoptOpenJDK-1.8.0_232-b09 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/testReport/ |
   | Max. process+thread count | 4300 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] sandeepvinayak commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
sandeepvinayak commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r543090129



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -64,6 +65,9 @@
   public static final String HBASE_REPLICATION_PEER_BASE_CONFIG =
     "hbase.replication.peer.base.config";
 
+  public static final String HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG =

Review comment:
       @bharathv do you mean setting k1="" if we want to remove k1 configuration?
   Just setting "" doesn't really communicate the program what we want to remove because there might be other other configs added from outside of hbase-site. 




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



[GitHub] [hbase] gjacoby126 commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r544691966



##########
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 testBasePeerConfigsRemovalForReplicationPeer()
+    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 testRemoveBasePeerConfigWithoutExistingConfigForReplicationPeer()
+    throws Exception {
+    LOG.info("testBasePeerConfigsForPeerMutations");
+    String firstCustomPeerConfigKey = "hbase.xxx.custom_config";
+
+    try {
+      baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+        firstCustomPeerConfigKey.concat("=").concat(""));
+      startMiniClusters(2);
+      addPeer("1", 0, 1);
+      Admin admin = utilities[0].getAdmin();
+
+      // Validates base configs 1 is present for both peer.

Review comment:
       nit: comment appears to be incorrect




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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-744702928


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 31s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 21s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   4m 22s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 56s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 51s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 24s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 14s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   4m  7s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 58s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 58s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m 52s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 22s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  6s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 26s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 134m  7s |  hbase-server in the patch passed.  |
   |  |   | 167m 50s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux c5b125e7694e 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Default Java | AdoptOpenJDK-11.0.6+10 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/testReport/ |
   | Max. process+thread count | 4362 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/1/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] virajjasani commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-746356621


   I think this approach of removing key if `{"key"=""}` is provided as updated peer base config sounds good one.
   
   > @virajjasani Do you think if it is standard to have conf upgrade before the HBase rolling upgrade? If not, we might need to rework on updating the peer configs
   
   For rolling upgrade, updating configs in new bits is common before performing rolling restart of master and RS (which can also update config symlinks e.g /etc/conf/hbase), hence this patch should fit well for upgrades IMHO.
   
   +1 for the PR overall once @bharathv and @gjacoby126 's remaining comments are addressed.


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



[GitHub] [hbase] bharathv commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r543078007



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -64,6 +65,9 @@
   public static final String HBASE_REPLICATION_PEER_BASE_CONFIG =
     "hbase.replication.peer.base.config";
 
+  public static final String HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG =

Review comment:
       I think this approach can be better, it seems confusing that one would add a configuration to remove another configuration?  (and the community is generally against adding such esoteric configs)
   
   What happens if someone forgets to undo this and that has some adverse affect after a next restart..
   
   Why not just consider "" config as a special case and then reset the configs in the copiedConfigBuilder?
   
   Something like
   
   initially someone set it k1=v1;k2=v2.... 
   then they can set it to "" and restart master
   
   detect that "" has been set (which is different from what we already have) and remove whatever we already have populated?
   
   To avoid config divergence, we can disallow hbase.replication.peer.base.config to be done via updateConfig RPC (throw some sensible exception)..

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -455,36 +458,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
 
   /**
    * 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". Original value
-   * of conf is retained if already present in ReplicationPeerConfig.
+   * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1".
    *
    * @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, "");
+    String removeBasePeerConfigs = conf.get(HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG, "");
     ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
       newBuilder(receivedPeerConfig);
-    Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
 
+    // remove the peer configurations specified in the conf

Review comment:
       Sorry just got to this PR. Thats right, master branch replication coordination is different from branch-1. This was specifically taken into consideration in the original PR that added this feature (see the commit message for branch-1).. 




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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745235437


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m  7s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  4s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 12s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   4m  0s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 58s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 55s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 24s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 16s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   4m 14s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 57s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 57s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   7m  6s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 25s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  8s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 43s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 139m 14s |  hbase-server in the patch passed.  |
   |  |   | 174m  5s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux bc82d7562ba1 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Default Java | AdoptOpenJDK-11.0.6+10 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/testReport/ |
   | Max. process+thread count | 4003 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745166963


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 29s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 27s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   1m 41s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   3m 22s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 28s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   0m 26s |  hbase-client: The patch generated 0 new + 0 unchanged - 3 fixed = 0 total (was 3)  |
   | +1 :green_heart: |  checkstyle  |   0m 12s |  hbase-replication: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)  |
   | +1 :green_heart: |  checkstyle  |   1m  2s |  hbase-server: The patch generated 0 new + 0 unchanged - 12 fixed = 0 total (was 12)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  hadoopcheck  |  17m  2s |  Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.  |
   | +1 :green_heart: |  spotbugs  |   3m 53s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 38s |  The patch does not generate ASF License warnings.  |
   |  |   |  43m 41s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux a8860306be95 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Max. process+thread count | 94 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/console |
   | versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-746820559


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 30s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 25s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 43s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   1m 45s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   3m 24s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 25s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   0m 26s |  hbase-client: The patch generated 0 new + 0 unchanged - 3 fixed = 0 total (was 3)  |
   | +1 :green_heart: |  checkstyle  |   0m 13s |  hbase-replication: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)  |
   | +1 :green_heart: |  checkstyle  |   1m  6s |  hbase-server: The patch generated 0 new + 0 unchanged - 12 fixed = 0 total (was 12)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  hadoopcheck  |  17m  5s |  Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.  |
   | +1 :green_heart: |  spotbugs  |   3m 55s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 38s |  The patch does not generate ASF License warnings.  |
   |  |   |  44m 40s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux bc1c11456190 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 74d68180e6 |
   | Max. process+thread count | 94 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/console |
   | versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745143339


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 12s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  4s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 31s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   4m 10s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 54s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 51s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 24s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 16s |  Maven dependency ordering for patch  |
   | -1 :x: |  mvninstall  |   1m 17s |  root in the patch failed.  |
   | -1 :x: |  compile  |   0m 14s |  hbase-client in the patch failed.  |
   | -1 :x: |  compile  |   0m 18s |  hbase-replication in the patch failed.  |
   | -1 :x: |  compile  |   0m 42s |  hbase-server in the patch failed.  |
   | -0 :warning: |  javac  |   0m 14s |  hbase-client in the patch failed.  |
   | -0 :warning: |  javac  |   0m 18s |  hbase-replication in the patch failed.  |
   | -0 :warning: |  javac  |   0m 42s |  hbase-server in the patch failed.  |
   | -1 :x: |  shadedjars  |   3m  5s |  patch has 10 errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 22s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  |   0m 15s |  hbase-client in the patch failed.  |
   | -1 :x: |  unit  |   0m 18s |  hbase-replication in the patch failed.  |
   | -1 :x: |  unit  |   0m 43s |  hbase-server in the patch failed.  |
   |  |   |  25m 54s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 22f6d3e346f5 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Default Java | AdoptOpenJDK-11.0.6+10 |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-mvninstall-root.txt |
   | compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-compile-hbase-client.txt |
   | compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-compile-hbase-replication.txt |
   | compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-compile-hbase-server.txt |
   | javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-compile-hbase-client.txt |
   | javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-compile-hbase-replication.txt |
   | javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-compile-hbase-server.txt |
   | shadedjars | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-shadedjars.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-client.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-replication.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/testReport/ |
   | Max. process+thread count | 101 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] sandeepvinayak commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
sandeepvinayak commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r544037912



##########
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:
       Yes, these are check style fixes, since I changed the file, I though of fixing these. 




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



[GitHub] [hbase] virajjasani closed pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
virajjasani closed pull request #2778:
URL: https://github.com/apache/hbase/pull/2778


   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-747004913


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m  8s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 22s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   4m 12s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 54s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 40s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 25s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 16s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   4m  3s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 54s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 54s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m 42s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 24s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  7s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 40s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 136m 49s |  hbase-server in the patch passed.  |
   |  |   | 171m 15s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 10578df0d917 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 74d68180e6 |
   | Default Java | AdoptOpenJDK-11.0.6+10 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/testReport/ |
   | Max. process+thread count | 4285 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-747226099


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 42s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 28s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   4m 16s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 57s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   8m 44s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 24s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 17s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   4m 20s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 56s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 56s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   8m 26s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 23s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m 15s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 29s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 144m  8s |  hbase-server in the patch passed.  |
   |  |   | 182m 26s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 43912df6e9cf 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 15d229eb35 |
   | Default Java | AdoptOpenJDK-1.8.0_232-b09 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/testReport/ |
   | Max. process+thread count | 4424 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] sandeepvinayak commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
sandeepvinayak commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r543049473



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -455,36 +458,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
 
   /**
    * 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". Original value
-   * of conf is retained if already present in ReplicationPeerConfig.
+   * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1".
    *
    * @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, "");
+    String removeBasePeerConfigs = conf.get(HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG, "");
     ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
       newBuilder(receivedPeerConfig);
-    Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
 
+    // remove the peer configurations specified in the conf

Review comment:
       That's a good point. Ideally IMO, the peer configuration should also not be added from the region server restart. We should have a better home for this (may be master restart?). 
   
   @virajjasani  Do you think if it is standard to have conf upgrade before the HBase rolling upgrade? If not, we might need to rework on updating the peer configs
   
   




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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-747006652


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 29s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 33s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 34s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 42s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 29s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 16s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 17s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 24s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 42s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 42s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m 34s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 14s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  5s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 26s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 140m 52s |  hbase-server in the patch passed.  |
   |  |   | 172m 15s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux f696c418da28 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 74d68180e6 |
   | Default Java | AdoptOpenJDK-1.8.0_232-b09 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/testReport/ |
   | Max. process+thread count | 4827 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/5/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745142386


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 31s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 14s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 22s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 46s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   7m 41s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 16s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 15s |  Maven dependency ordering for patch  |
   | -1 :x: |  mvninstall  |   1m  5s |  root in the patch failed.  |
   | -1 :x: |  compile  |   0m 13s |  hbase-client in the patch failed.  |
   | -1 :x: |  compile  |   0m 18s |  hbase-replication in the patch failed.  |
   | -1 :x: |  compile  |   0m 39s |  hbase-server in the patch failed.  |
   | -0 :warning: |  javac  |   0m 13s |  hbase-client in the patch failed.  |
   | -0 :warning: |  javac  |   0m 18s |  hbase-replication in the patch failed.  |
   | -0 :warning: |  javac  |   0m 39s |  hbase-server in the patch failed.  |
   | -1 :x: |  shadedjars  |   2m 57s |  patch has 10 errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 13s |  the patch passed  |
   ||| _ Other Tests _ |
   | -1 :x: |  unit  |   0m 14s |  hbase-client in the patch failed.  |
   | -1 :x: |  unit  |   0m 17s |  hbase-replication in the patch failed.  |
   | -1 :x: |  unit  |   0m 39s |  hbase-server in the patch failed.  |
   |  |   |  23m 56s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 5015f5776a83 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Default Java | AdoptOpenJDK-1.8.0_232-b09 |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-mvninstall-root.txt |
   | compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-compile-hbase-client.txt |
   | compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-compile-hbase-replication.txt |
   | compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-compile-hbase-server.txt |
   | javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-compile-hbase-client.txt |
   | javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-compile-hbase-replication.txt |
   | javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-compile-hbase-server.txt |
   | shadedjars | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-shadedjars.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-client.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-replication.txt |
   | unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/testReport/ |
   | Max. process+thread count | 88 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] sandeepvinayak commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
sandeepvinayak commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r543090129



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -64,6 +65,9 @@
   public static final String HBASE_REPLICATION_PEER_BASE_CONFIG =
     "hbase.replication.peer.base.config";
 
+  public static final String HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG =

Review comment:
       @bharathv do you mean setting k1="" if we want to remove k1 configuration?
   Just setting "" doesn't really communicate the program what we want to remove because there might be other configs added from outside of hbase-site. 




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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-747184441


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 28s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 26s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 40s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   1m 43s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   3m 22s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 25s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   0m 27s |  hbase-client: The patch generated 0 new + 0 unchanged - 3 fixed = 0 total (was 3)  |
   | +1 :green_heart: |  checkstyle  |   0m 12s |  hbase-replication: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)  |
   | +1 :green_heart: |  checkstyle  |   1m  3s |  hbase-server: The patch generated 0 new + 0 unchanged - 12 fixed = 0 total (was 12)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  hadoopcheck  |  17m 18s |  Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.  |
   | +1 :green_heart: |  spotbugs  |   3m 57s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 38s |  The patch does not generate ASF License warnings.  |
   |  |   |  44m 37s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux 8149338bd264 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 15d229eb35 |
   | Max. process+thread count | 94 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/console |
   | versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745055055


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 37s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 24s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 49s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   1m 52s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   3m 41s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 47s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   0m 27s |  The patch passed checkstyle in hbase-client  |
   | +1 :green_heart: |  checkstyle  |   0m 10s |  hbase-replication: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)  |
   | +1 :green_heart: |  checkstyle  |   1m  5s |  The patch passed checkstyle in hbase-server  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  hadoopcheck  |  18m 29s |  Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.  |
   | +1 :green_heart: |  spotbugs  |   3m 58s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 35s |  The patch does not generate ASF License warnings.  |
   |  |   |  46m 44s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/2/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux 531bbba705d0 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Max. process+thread count | 94 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/2/console |
   | versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-747219934


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 29s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  4s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 24s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   4m 13s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 53s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 41s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 26s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 16s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   4m  0s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 54s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 54s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m 43s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 22s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  9s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 26s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 134m 39s |  hbase-server in the patch passed.  |
   |  |   | 168m 18s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 928a547f1099 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 15d229eb35 |
   | Default Java | AdoptOpenJDK-11.0.6+10 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/testReport/ |
   | Max. process+thread count | 4157 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/6/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745140675


   :broken_heart: **-1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m 15s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 13s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 36s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   1m 47s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   3m 33s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 14s |  Maven dependency ordering for patch  |
   | -1 :x: |  mvninstall  |   1m  2s |  root in the patch failed.  |
   | -0 :warning: |  checkstyle  |   0m 24s |  hbase-client: The patch generated 3 new + 3 unchanged - 0 fixed = 6 total (was 3)  |
   | +1 :green_heart: |  checkstyle  |   0m  9s |  hbase-replication: The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)  |
   | +1 :green_heart: |  checkstyle  |   0m 59s |  hbase-server: The patch generated 0 new + 0 unchanged - 12 fixed = 0 total (was 12)  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | -1 :x: |  hadoopcheck  |   1m 12s |  The patch causes 10 errors with Hadoop v3.1.2.  |
   | -1 :x: |  hadoopcheck  |   2m 20s |  The patch causes 10 errors with Hadoop v3.2.1.  |
   | -1 :x: |  hadoopcheck  |   3m 31s |  The patch causes 10 errors with Hadoop v3.3.0.  |
   | -1 :x: |  spotbugs  |   0m 11s |  hbase-client in the patch failed.  |
   | -1 :x: |  spotbugs  |   0m 16s |  hbase-replication in the patch failed.  |
   | -1 :x: |  spotbugs  |   0m 37s |  hbase-server in the patch failed.  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 29s |  The patch does not generate ASF License warnings.  |
   |  |   |  20m  4s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux 8308a0e0e143 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | mvninstall | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-mvninstall-root.txt |
   | checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-javac-3.1.2.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-javac-3.2.1.txt |
   | hadoopcheck | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-javac-3.3.0.txt |
   | spotbugs | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-spotbugs-hbase-client.txt |
   | spotbugs | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-spotbugs-hbase-replication.txt |
   | spotbugs | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/artifact/yetus-general-check/output/patch-spotbugs-hbase-server.txt |
   | Max. process+thread count | 94 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/3/console |
   | versions | git=2.17.1 maven=3.6.3 spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] gjacoby126 commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r542727432



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -455,36 +458,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
 
   /**
    * 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". Original value
-   * of conf is retained if already present in ReplicationPeerConfig.
+   * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1".
    *
    * @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, "");
+    String removeBasePeerConfigs = conf.get(HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG, "");
     ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
       newBuilder(receivedPeerConfig);
-    Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
 
+    // remove the peer configurations specified in the conf
+    if (removeBasePeerConfigs.length() != 0) {
+      List<String> removeBasePeerConfigList = Splitter.on(';').trimResults()
+        .omitEmptyStrings().splitToList(removeBasePeerConfigs);
+      for (String peerConfigToRemove : removeBasePeerConfigList) {
+        copiedPeerConfigBuilder.removeConfiguration(peerConfigToRemove);
+      }
+    }
+
+    Map<String, String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
+    String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, "");

Review comment:
       What happens if I try to add a peer config setting and remove the same peer config setting in the same Configuration object? Looks like the add just silently wins? I'd think that should produce an exception (or at the very least a log). 

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -455,36 +458,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
 
   /**
    * 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". Original value
-   * of conf is retained if already present in ReplicationPeerConfig.
+   * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1".
    *
    * @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, "");
+    String removeBasePeerConfigs = conf.get(HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG, "");
     ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
       newBuilder(receivedPeerConfig);
-    Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
 
+    // remove the peer configurations specified in the conf

Review comment:
       Because configuration settings are often from hbase-site.xml, and hbase-site.xml can be different on different region servers (e.g when upgrading a cluster), it would seem like you could get into some unwelcome race conditions with this approach. 
   
   Let's say that you're doing a rolling restart from Version 1 to Version 2. Version 1 has a config C == true, Version 2 tries to remove C. 
   
   1. Server 1 is upgraded to Version 2 and restarts, and removes C from the peer config
   2. Server 2 is still on Version 1 and is restarted for an unrelated reason (maybe it crashes and is restarted). It sees that C is not in the peer config, and recreates it. 
   
   Assuming that you upgraded hbase-site.xml everywhere and restarted, the cluster would _eventually_ converge on the right answer, but there's a lot of opportunity for the config to flicker back and forth in the meantime. Depending on the config, that could have incorrect or unpredictable results 




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



[GitHub] [hbase] Apache-HBase commented on pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#issuecomment-745235770


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 29s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 23s |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |   3m 41s |  master passed  |
   | +1 :green_heart: |  compile  |   1m 40s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m 34s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 15s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 16s |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   3m 39s |  the patch passed  |
   | +1 :green_heart: |  compile  |   1m 43s |  the patch passed  |
   | +1 :green_heart: |  javac  |   1m 43s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m 44s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   1m 16s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  7s |  hbase-client in the patch passed.  |
   | +1 :green_heart: |  unit  |   0m 27s |  hbase-replication in the patch passed.  |
   | +1 :green_heart: |  unit  | 143m 10s |  hbase-server in the patch passed.  |
   |  |   | 174m 51s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2778 |
   | JIRA Issue | HBASE-25383 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 2801c2fdf65b 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / d50816fe44 |
   | Default Java | AdoptOpenJDK-1.8.0_232-b09 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/testReport/ |
   | Max. process+thread count | 4777 (vs. ulimit of 30000) |
   | modules | C: hbase-client hbase-replication hbase-server U: . |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2778/4/console |
   | versions | git=2.17.1 maven=3.6.3 |
   | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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



[GitHub] [hbase] bharathv commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org



[GitHub] [hbase] sandeepvinayak commented on a change in pull request #2778: HBASE-25383: Ability to update and remove peer base config

Posted by GitBox <gi...@apache.org>.
sandeepvinayak commented on a change in pull request #2778:
URL: https://github.com/apache/hbase/pull/2778#discussion_r543074326



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -455,36 +458,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
 
   /**
    * 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". Original value
-   * of conf is retained if already present in ReplicationPeerConfig.
+   * Expected format for this hbase configuration is "k1=v1;k2=v2,v2_1".
    *
    * @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, "");
+    String removeBasePeerConfigs = conf.get(HBASE_REPLICATION_PEER_REMOVE_BASE_CONFIG, "");
     ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
       newBuilder(receivedPeerConfig);
-    Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
 
+    // remove the peer configurations specified in the conf

Review comment:
       Also, looking at the `master` branch code, looks like the RS restart has nothing to do with this update config. Only master restart is able to update configs. 




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