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/08/19 19:54:48 UTC
[GitHub] [hbase] ankitjain64 opened a new pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
ankitjain64 opened a new pull request #2284:
URL: https://github.com/apache/hbase/pull/2284
JIRA: https://issues.apache.org/jira/browse/HBASE-24764
JIRA Description:
Currently, if a user needs to apply some common peer configs to all the default replication peers, the only way is to execute update_peer_config via CLI which requires manual intervention and can be tedious in case of large deployment fleet.
As part of this JIRA, we plan to add the support to have default replication peer configs as part of hbase-site.xml like hbase.replication.peer.default.config="k1=v1;k2=v2.." which can be just applied by a rolling restart. Example below:
<property>
<name>hbase.replication.peer.default.configs</name>
<value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.rpc.protection=abc;hbase.xxx.custom_property=123</value>
</property>
This will be empty by default, but one can override to have default configs in place.
The final peer configuration would be a merge of this default config + whatever users override during the peer creation/update (if any).
Related Jira: https://issues.apache.org/jira/browse/HBASE-17543. HBASE-17543 added the support to add the WALEntryFilters to default endpoint via peer configuration. By this new Jira we are extending the support to update peer configs via hbase-site.xml.
----------------------------------------------------------------
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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r476944550
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,52 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+
+ String customPeerConfigSecondKey = "hbase.xxx.custom_second_config";
+ String customPeerConfigSecondValue = "testSecond";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+ // custom config not present
+ assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue).concat(";").
+ concat(customPeerConfigSecondKey).concat("=").concat(customPeerConfigSecondValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
+
+ assertEquals(customPeerConfigValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigKey));
+ assertEquals(customPeerConfigSecondValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigSecondKey));
+ }
+
+ @Test
+ public void testDefaultReplicationPeerConfigOverrideIfAlreadySet(){
Review comment:
Added the tests for admin code paths.
The behavior after this patch is that a peer config will only be updated by `configuration` object if that new configuration was not present in `ReplicationPeerConfig`. If it was already present then old value is retained and values from `configuration` object won't make any changes.
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-680734154
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 42s | 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 | 4m 45s | master passed |
| +1 :green_heart: | checkstyle | 1m 57s | master passed |
| +1 :green_heart: | spotbugs | 4m 23s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 33s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 3s | the patch passed |
| +1 :green_heart: | whitespace | 0m 1s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 15m 11s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 4m 1s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 38s | The patch does not generate ASF License warnings. |
| | | 47m 8s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux 07b9fd8e4608 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 / 01cf60067c |
| Max. process+thread count | 95 (vs. ulimit of 12500) |
| 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-2284/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-682252799
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 49s | 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 26s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 49s | master passed |
| +1 :green_heart: | compile | 2m 24s | master passed |
| +1 :green_heart: | shadedjars | 8m 10s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 31s | hbase-client in master failed. |
| -0 :warning: | javadoc | 0m 18s | hbase-replication in master failed. |
| -0 :warning: | javadoc | 0m 47s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 24s | the patch passed |
| +1 :green_heart: | compile | 2m 35s | the patch passed |
| +1 :green_heart: | javac | 2m 35s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 36s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 30s | hbase-client in the patch failed. |
| -0 :warning: | javadoc | 0m 19s | hbase-replication in the patch failed. |
| -0 :warning: | javadoc | 0m 51s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 41s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 43s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 215m 38s | hbase-server in the patch passed. |
| | | 259m 10s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 2e892d7eb2fa 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 / 047e0618d2 |
| Default Java | 2020-01-14 |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/testReport/ |
| Max. process+thread count | 2891 (vs. ulimit of 12500) |
| 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-2284/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r474346612
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
+
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
Review comment:
>> so you could choose a delimiter that was outside of the constraint set
Are there any current constraints regarding the convention on peer values that I can refer to?
I looked around and since there can be multiple values of a particular peer config that are generally delimited by `,` so I decided to use `;` for delimiting different peer configs. But I am happy to change this if it can cause any problems in your opinion. Thanks
----------------------------------------------------------------
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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r476939079
##########
File path: hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeers.java
##########
@@ -144,7 +145,12 @@ private ReplicationPeerImpl createPeer(String peerId) throws ReplicationExceptio
SyncReplicationState syncReplicationState = peerStorage.getPeerSyncReplicationState(peerId);
SyncReplicationState newSyncReplicationState =
peerStorage.getPeerNewSyncReplicationState(peerId);
+
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(this.conf, peerConfig);
+ peerStorage.updatePeerConfig(peerId,updatedPeerConfig);
Review comment:
Yes, that completely makes sense. Updated in the latest commit.
----------------------------------------------------------------
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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r478709945
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -441,6 +442,41 @@ public void testCyclicReplication3() throws Exception {
}
}
+ /**
+ * Tests that base replication peer configs are applied on peer creation
+ * and the is overriden if updated as part of updateReplicationPeerConfig()
+ *
+ */
+ @Test
+ public void testBasePeerConfigsAppliedOnPeerCreationAndUpdate()
+ throws Exception {
+ LOG.info("testBasePeerConfigsAppliedOnPeerCreation");
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+ try {
+ baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+ startMiniClusters(2);
+ addPeer("1", 0, 1);
+ Admin admin = utilities[0].getAdmin();
+
+ Assert.assertEquals(customPeerConfigValue, admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfig.
+ newBuilder(admin.getReplicationPeerConfig("1")).
+ putConfiguration(customPeerConfigKey,customPeerConfigUpdatedValue).build();
+ admin.updateReplicationPeerConfig("1", updatedReplicationPeerConfig);
Review comment:
Looks like hbase does not provide the support for deleting a peer configuration, we can only update the value after a configuration is added. But I have added the test case for the scenario that new base config gets picked up after hbase restart.
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-680795098
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 29s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | 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 37s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 23s | master passed |
| +1 :green_heart: | compile | 1m 54s | master passed |
| +1 :green_heart: | shadedjars | 6m 39s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 28s | hbase-client in master failed. |
| -0 :warning: | javadoc | 0m 17s | hbase-replication in master failed. |
| -0 :warning: | javadoc | 0m 39s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 33s | the patch passed |
| +1 :green_heart: | compile | 2m 11s | the patch passed |
| +1 :green_heart: | javac | 2m 11s | the patch passed |
| +1 :green_heart: | shadedjars | 7m 22s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 25s | hbase-client in the patch failed. |
| -0 :warning: | javadoc | 0m 17s | hbase-replication in the patch failed. |
| -0 :warning: | javadoc | 0m 39s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 16s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 26s | hbase-replication in the patch passed. |
| -1 :x: | unit | 135m 28s | hbase-server in the patch failed. |
| | | 170m 46s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 85f2f690d921 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 / 01cf60067c |
| Default Java | 2020-01-14 |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/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-2284/3/testReport/ |
| Max. process+thread count | 4160 (vs. ulimit of 12500) |
| 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-2284/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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] saintstack commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r474300228
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -60,6 +60,7 @@
public final class ReplicationPeerConfigUtil {
private static final Logger LOG = LoggerFactory.getLogger(ReplicationPeerConfigUtil.class);
+ public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG= "hbase.replication.peer.default.config";
Review comment:
Is this 'default' or 'base' configuration for all peers?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
+
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){
Review comment:
See how rest of code has space between keyword and brackets as in 'if (' rather than 'if('.... you do this a few times in this PR.
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,40 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+ // custom config not present
+ assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_DEFAULT_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.addDefaultPeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
Review comment:
Long lines? 100chars is max.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
+
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
Review comment:
';' is safe character to use as delimiter for sure? Will never be part of a config value? Are the constraint on peer values at all so you could choose a delimiter that was outside of the constraint set?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] ankitjain64 commented on pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-677976677
> Is there any place this nice new facility is documented? Thanks.
I have updated the PR description and also the JIRA about the enhancement that we want to bring as part of this item. Please let me know if you have any questions/concerns will be happy to answer. Thanks
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-682263818
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 23s | 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 28s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 1s | master passed |
| +1 :green_heart: | checkstyle | 1m 49s | master passed |
| +1 :green_heart: | spotbugs | 3m 32s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 43s | the patch passed |
| -0 :warning: | checkstyle | 0m 27s | hbase-client: The patch generated 1 new + 2 unchanged - 0 fixed = 3 total (was 2) |
| -0 :warning: | checkstyle | 0m 10s | hbase-replication: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 12m 10s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 4m 0s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 31s | The patch does not generate ASF License warnings. |
| | | 40m 32s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux ad1d9d3a56eb 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 / 047e0618d2 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-general-check/output/diff-checkstyle-hbase-replication.txt |
| Max. process+thread count | 84 (vs. ulimit of 12500) |
| 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-2284/5/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r474343617
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -60,6 +60,7 @@
public final class ReplicationPeerConfigUtil {
private static final Logger LOG = LoggerFactory.getLogger(ReplicationPeerConfigUtil.class);
+ public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG= "hbase.replication.peer.default.config";
Review comment:
Updated in the latest commit.
----------------------------------------------------------------
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] ankitjain64 commented on pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-676630230
@bharathv : Can you please have a look ? Thanks
----------------------------------------------------------------
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] saintstack commented on pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
saintstack commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-677934075
Is there any place this nice new facility is documented? Thanks.
----------------------------------------------------------------
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 pull request #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
bharathv commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-689685498
@saintstack sorry to bother you, just checking to see if you have any pending comments. Will go ahead and merge this (tomorrow) if you are busy and won't be able to take another pass. Ankit can do a follow up if you have any comments later. Thanks.
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-682170122
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 22s | 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 | 4m 0s | master passed |
| +1 :green_heart: | checkstyle | 1m 49s | master passed |
| +1 :green_heart: | spotbugs | 3m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 46s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 49s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 12m 12s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 4m 4s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 31s | The patch does not generate ASF License warnings. |
| | | 40m 57s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux dbf446341bc4 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 / 047e0618d2 |
| Max. process+thread count | 84 (vs. ulimit of 12500) |
| 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-2284/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r478708944
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +219,51 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testBaseReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+
+ String customPeerConfigSecondKey = "hbase.xxx.custom_second_config";
+ String customPeerConfigSecondValue = "testSecond";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+ // custom config not present
+ assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue).concat(";").
+ concat(customPeerConfigSecondKey).concat("=").concat(customPeerConfigSecondValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
+
+ assertEquals(customPeerConfigValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigKey));
+ assertEquals(customPeerConfigSecondValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigSecondKey));
+ }
+
+ @Test
+ public void testBaseReplicationPeerConfigDoesNotOverrideIfAlreadySet(){
+
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigOldValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = ReplicationPeerConfig.
+ newBuilder(getConfig(1))
+ .putConfiguration(customPeerConfigKey,customPeerConfigOldValue).build();
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigUpdatedValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
Review comment:
Sure. Merged the test in latest commit.
----------------------------------------------------------------
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 #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-683229640
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 28s | 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 24s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 0s | master passed |
| +1 :green_heart: | compile | 1m 52s | master passed |
| +1 :green_heart: | shadedjars | 6m 34s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 25s | hbase-client in master failed. |
| -0 :warning: | javadoc | 0m 17s | hbase-replication in master failed. |
| -0 :warning: | javadoc | 0m 40s | hbase-server in master failed. |
||| _ 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 35s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 26s | hbase-client in the patch failed. |
| -0 :warning: | javadoc | 0m 16s | hbase-replication in the patch failed. |
| -0 :warning: | javadoc | 0m 40s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 10s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 27s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 130m 50s | hbase-server in the patch passed. |
| | | 163m 52s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| JIRA Issue | HBASE-24764 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 92bf62ea7018 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 / 7909e29de5 |
| Default Java | 2020-01-14 |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/testReport/ |
| Max. process+thread count | 3946 (vs. ulimit of 12500) |
| 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-2284/6/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 merged pull request #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
bharathv merged pull request #2284:
URL: https://github.com/apache/hbase/pull/2284
----------------------------------------------------------------
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 #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-683236670
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 33s | 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 26s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 29s | master passed |
| +1 :green_heart: | compile | 1m 59s | master passed |
| +1 :green_heart: | shadedjars | 8m 14s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 22s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 21s | the patch passed |
| +1 :green_heart: | compile | 2m 3s | the patch passed |
| +1 :green_heart: | javac | 2m 3s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 46s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 39s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 40s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 52s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 205m 9s | hbase-server in the patch passed. |
| | | 245m 0s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| JIRA Issue | HBASE-24764 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 4f47b2c8ebf3 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 / 7909e29de5 |
| Default Java | 1.8.0_232 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/testReport/ |
| Max. process+thread count | 2898 (vs. ulimit of 12500) |
| 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-2284/6/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-676818729
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 3m 3s | 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 43s | master passed |
| +1 :green_heart: | compile | 1m 57s | master passed |
| +1 :green_heart: | shadedjars | 7m 0s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 35s | hbase-client in master failed. |
| -0 :warning: | javadoc | 0m 17s | hbase-replication in master failed. |
| -0 :warning: | javadoc | 0m 48s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 30s | the patch passed |
| +1 :green_heart: | compile | 2m 15s | the patch passed |
| +1 :green_heart: | javac | 2m 15s | the patch passed |
| +1 :green_heart: | shadedjars | 7m 20s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 31s | hbase-client in the patch failed. |
| -0 :warning: | javadoc | 0m 18s | hbase-replication in the patch failed. |
| -0 :warning: | javadoc | 0m 51s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 28s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 57s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 193m 51s | hbase-server in the patch passed. |
| | | 234m 10s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux a8cf5e915869 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 / 1164531d5a |
| Default Java | 2020-01-14 |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/testReport/ |
| Max. process+thread count | 3531 (vs. ulimit of 12500) |
| 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-2284/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r473434176
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
Review comment:
nit: Sample configuration block not needed. Its kind of obvious. Instead add the configuration value formatting to the javadoc of the config? (and also what the default is).
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -60,6 +60,7 @@
public final class ReplicationPeerConfigUtil {
private static final Logger LOG = LoggerFactory.getLogger(ReplicationPeerConfigUtil.class);
+ public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG= "hbase.replication.peer.default.config";
Review comment:
Make it private?
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,52 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+
+ String customPeerConfigSecondKey = "hbase.xxx.custom_second_config";
+ String customPeerConfigSecondValue = "testSecond";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+ // custom config not present
+ assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue).concat(";").
+ concat(customPeerConfigSecondKey).concat("=").concat(customPeerConfigSecondValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
+
+ assertEquals(customPeerConfigValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigKey));
+ assertEquals(customPeerConfigSecondValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigSecondKey));
+ }
+
+ @Test
+ public void testDefaultReplicationPeerConfigOverrideIfAlreadySet(){
Review comment:
I think we need more coverage for the following cases.
- Existing peer config gets the config override
- Admin code paths (for getPeerConfig, updatePeerConfig, etc) work well with the overlays (updating an existing / non-existing config etc)
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
Review comment:
nit: get(CONFIG, default)
##########
File path: hbase-replication/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeers.java
##########
@@ -144,7 +145,12 @@ private ReplicationPeerImpl createPeer(String peerId) throws ReplicationExceptio
SyncReplicationState syncReplicationState = peerStorage.getPeerSyncReplicationState(peerId);
SyncReplicationState newSyncReplicationState =
peerStorage.getPeerNewSyncReplicationState(peerId);
+
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(this.conf, peerConfig);
+ peerStorage.updatePeerConfig(peerId,updatedPeerConfig);
Review comment:
I was thinking this happens only in the master code paths. Ex: ReplicationPeerManager#create (for existing peers) or addPeer() for new peers etc. That way the configuration in storage remains consistent.
Doing from the RS code paths (ReplicationPeers) means that if different RS run with different configs it can result in a different final state (depending which RS does this RPC last). Also doing this from HMaster seems logical since this is more like an admin operation whereas RS based codepaths are just consumers of this config.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
Review comment:
Mind fixing the check-style issues from precommit? Bunch of overflows.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,50 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.base.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;
+ hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add base peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
Review comment:
nit: return value javadoc seems wrong.
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,52 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
Review comment:
Two tests can be merged into one.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
+
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
Review comment:
Also Splitter from Guava provides a clean API to do this parsing (couple of lines of code).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r478708730
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else null.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if (defaultPeerConfigs != null && defaultPeerConfigs.length() != 0) {
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
+
+ for (String defaultPeerConfig : defaultPeerConfigList) {
+ String[] configSplit = defaultPeerConfig.split("=");
+
+ if (configSplit != null && configSplit.length == 2) {
+ String configName = configSplit[0];
+ String configValue = configSplit[1];
+
+ // Only override if base config does not exist in existing peer configs
+ if (!peerConfigurations.containsKey(configName)) {
+ copiedPeerConfigBuilder.putConfiguration(configName,configValue);
+ isPeerConfigChanged = true;
+ }
+ }
+ }
+ }
+
+ return isPeerConfigChanged ? copiedPeerConfigBuilder.build() : null;
Review comment:
I thought it's not a good idea not make a zk call to update storage if there are no changes to peer configurations, that's why modified the return objects here. What do you think. ?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else null.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if (defaultPeerConfigs != null && defaultPeerConfigs.length() != 0) {
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
+
+ for (String defaultPeerConfig : defaultPeerConfigList) {
+ String[] configSplit = defaultPeerConfig.split("=");
+
+ if (configSplit != null && configSplit.length == 2) {
+ String configName = configSplit[0];
+ String configValue = configSplit[1];
+
+ // Only override if base config does not exist in existing peer configs
+ if (!peerConfigurations.containsKey(configName)) {
+ copiedPeerConfigBuilder.putConfiguration(configName,configValue);
+ isPeerConfigChanged = true;
+ }
+ }
+ }
+ }
+
+ return isPeerConfigChanged ? copiedPeerConfigBuilder.build() : null;
Review comment:
Done in latest commit.
----------------------------------------------------------------
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 #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r479576404
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
##########
@@ -232,6 +233,9 @@ public void addPeer(String peerId, ReplicationPeerConfig peerConfig, boolean ena
// this should be a retry, just return
return;
}
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,peerConfig);
+ peerConfig = updatedPeerConfig;
Review comment:
merge into a single line.
peerConfig = ReplicatinPeerConfigUtil..addPeer..IfNotPresent()
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +453,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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.
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig containing updated configs.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig) {
+ String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, null);
Review comment:
nit: Use the default as empty string? That way we can avoid null check in if, not a big deal, just to be consistent with other similar usages.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +453,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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.
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig containing updated configs.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig) {
+ String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
+
Review comment:
nit: Remove multiple extraneous new lines in this method.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
##########
@@ -546,9 +550,13 @@ public static ReplicationPeerManager create(ZKWatcher zk, Configuration conf, St
ConcurrentMap<String, ReplicationPeerDescription> peers = new ConcurrentHashMap<>();
for (String peerId : peerStorage.listPeerIds()) {
ReplicationPeerConfig peerConfig = peerStorage.getPeerConfig(peerId);
+
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
Review comment:
peerConfig = ReplicationpeerConfigUtil.....() (avoid unnecessary temp variable)
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
##########
@@ -546,9 +550,13 @@ public static ReplicationPeerManager create(ZKWatcher zk, Configuration conf, St
ConcurrentMap<String, ReplicationPeerDescription> peers = new ConcurrentHashMap<>();
for (String peerId : peerStorage.listPeerIds()) {
ReplicationPeerConfig peerConfig = peerStorage.getPeerConfig(peerId);
+
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,peerConfig);
+ peerStorage.updatePeerConfig(peerId,updatedPeerConfig);
Review comment:
nit: , updatedPeerConfig (space).
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/replication/ReplicationPeerManager.java
##########
@@ -232,6 +233,9 @@ public void addPeer(String peerId, ReplicationPeerConfig peerConfig, boolean ena
// this should be a retry, just return
return;
}
+ ReplicationPeerConfig updatedPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,peerConfig);
Review comment:
nit: config, peerConfig (space)
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +453,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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.
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig containing updated configs.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig) {
+ String basePeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG, null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> receivedPeerConfigMap = receivedPeerConfig.getConfiguration();
+
+ if (basePeerConfigs != null && basePeerConfigs.length() != 0) {
+
+ Map<String, String> basePeerConfigMap = Splitter.on(';').trimResults().omitEmptyStrings()
+ .withKeyValueSeparator("=").split(basePeerConfigs);
+
+ 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)) {
+ copiedPeerConfigBuilder.putConfiguration(configName,configValue);
Review comment:
nit:, configValue (space)
----------------------------------------------------------------
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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r476939509
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +218,52 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testDefaultReplicationPeerConfigIsAppliedIfNotAlreadySet(){
Review comment:
Since they were testing different behaviors I kept them as different, do you think we should still merge 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] Apache-HBase commented on pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-676669995
: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 27s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 35s | master passed |
| +1 :green_heart: | checkstyle | 1m 43s | master passed |
| +1 :green_heart: | spotbugs | 3m 23s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 23s | the patch passed |
| -0 :warning: | checkstyle | 0m 26s | hbase-client: The patch generated 6 new + 2 unchanged - 0 fixed = 8 total (was 2) |
| -0 :warning: | checkstyle | 0m 12s | hbase-replication: The patch generated 6 new + 0 unchanged - 0 fixed = 6 total (was 0) |
| -0 :warning: | checkstyle | 1m 3s | hbase-server: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 5s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 3m 53s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 39s | The patch does not generate ASF License warnings. |
| | | 37m 56s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux 68e4f1df9b5a 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 / 1164531d5a |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-replication.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 94 (vs. ulimit of 12500) |
| 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-2284/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-680491158
: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 25s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 51s | master passed |
| +1 :green_heart: | checkstyle | 1m 53s | master passed |
| +1 :green_heart: | spotbugs | 3m 49s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 28s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 45s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 40s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 3m 52s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 38s | The patch does not generate ASF License warnings. |
| | | 40m 7s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux d86ef4b42b75 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 / 01cf60067c |
| Max. process+thread count | 94 (vs. ulimit of 12500) |
| 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-2284/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-682250024
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 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 31s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 31s | master passed |
| +1 :green_heart: | compile | 2m 1s | master passed |
| +1 :green_heart: | shadedjars | 7m 46s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 27s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 19s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 42s | the patch passed |
| +1 :green_heart: | compile | 2m 2s | the patch passed |
| +1 :green_heart: | javac | 2m 2s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 18s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 20s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 39s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 223m 40s | hbase-server in the patch passed. |
| | | 262m 17s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux a74898a6bdde 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 / 047e0618d2 |
| Default Java | 1.8.0_232 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/4/testReport/ |
| Max. process+thread count | 3310 (vs. ulimit of 12500) |
| 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-2284/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r476938656
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -60,6 +60,7 @@
public final class ReplicationPeerConfigUtil {
private static final Logger LOG = LoggerFactory.getLogger(ReplicationPeerConfigUtil.class);
+ public static final String HBASE_REPLICATION_PEER_DEFAULT_CONFIG= "hbase.replication.peer.default.config";
Review comment:
This is also getting referenced in other classes for testing, so kept it public.
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-680711097
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 16s | 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 25s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 30s | master passed |
| +1 :green_heart: | compile | 2m 24s | master passed |
| +1 :green_heart: | shadedjars | 7m 39s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 30s | hbase-client in master failed. |
| -0 :warning: | javadoc | 0m 17s | hbase-replication in master failed. |
| -0 :warning: | javadoc | 0m 45s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 57s | the patch passed |
| +1 :green_heart: | compile | 2m 0s | the patch passed |
| +1 :green_heart: | javac | 2m 0s | the patch passed |
| +1 :green_heart: | shadedjars | 7m 18s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 25s | hbase-client in the patch failed. |
| -0 :warning: | javadoc | 0m 16s | hbase-replication in the patch failed. |
| -0 :warning: | javadoc | 0m 41s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 17s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 37s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 222m 8s | hbase-server in the patch passed. |
| | | 262m 44s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux b1bfe87a1bb9 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 / 01cf60067c |
| Default Java | 2020-01-14 |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/testReport/ |
| Max. process+thread count | 3014 (vs. ulimit of 12500) |
| 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-2284/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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] ankitjain64 commented on pull request #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-686641195
@saintstack Can you please have a look at this patch whenever you get a chance? I have addressed all your comments. Thanks
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-682318041
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 34s | 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 24s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 50s | master passed |
| +1 :green_heart: | compile | 2m 6s | master passed |
| +1 :green_heart: | shadedjars | 8m 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 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 55s | the patch passed |
| +1 :green_heart: | compile | 1m 39s | the patch passed |
| +1 :green_heart: | javac | 1m 39s | the patch passed |
| +1 :green_heart: | shadedjars | 7m 10s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 10s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 12s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 36s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 219m 27s | hbase-server in the patch passed. |
| | | 256m 15s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 9aa71338b9b3 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 / 047e0618d2 |
| Default Java | 1.8.0_232 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/testReport/ |
| Max. process+thread count | 3065 (vs. ulimit of 12500) |
| 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-2284/5/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: HBASE-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-683213415
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 5s | 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 28s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 52s | master passed |
| +1 :green_heart: | checkstyle | 1m 50s | master passed |
| +1 :green_heart: | spotbugs | 3m 34s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 44s | the patch passed |
| -0 :warning: | checkstyle | 0m 28s | hbase-client: The patch generated 1 new + 2 unchanged - 0 fixed = 3 total (was 2) |
| -0 :warning: | checkstyle | 0m 11s | hbase-replication: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 12m 19s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 4m 4s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 32s | The patch does not generate ASF License warnings. |
| | | 41m 39s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| JIRA Issue | HBASE-24764 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
| uname | Linux 5b76a2931543 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 / 7909e29de5 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/6/artifact/yetus-general-check/output/diff-checkstyle-hbase-replication.txt |
| Max. process+thread count | 84 (vs. ulimit of 12500) |
| 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-2284/6/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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] ankitjain64 commented on a change in pull request #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
ankitjain64 commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r478708730
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else null.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if (defaultPeerConfigs != null && defaultPeerConfigs.length() != 0) {
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
+
+ for (String defaultPeerConfig : defaultPeerConfigList) {
+ String[] configSplit = defaultPeerConfig.split("=");
+
+ if (configSplit != null && configSplit.length == 2) {
+ String configName = configSplit[0];
+ String configValue = configSplit[1];
+
+ // Only override if base config does not exist in existing peer configs
+ if (!peerConfigurations.containsKey(configName)) {
+ copiedPeerConfigBuilder.putConfiguration(configName,configValue);
+ isPeerConfigChanged = true;
+ }
+ }
+ }
+ }
+
+ return isPeerConfigChanged ? copiedPeerConfigBuilder.build() : null;
Review comment:
I thought it's not a good idea not make a zk call to update storage if there are no changes to peer configurations, that's why modified the return objects here. What do you think. ?
----------------------------------------------------------------
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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-680827065
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 8s | 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 29s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 20s | master passed |
| +1 :green_heart: | compile | 2m 15s | master passed |
| +1 :green_heart: | shadedjars | 9m 27s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 59s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 19s | 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 | 7m 13s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 12s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 7s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 41s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 205m 10s | hbase-server in the patch passed. |
| | | 247m 8s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d2d8c863bc33 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 / 01cf60067c |
| Default Java | 1.8.0_232 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/3/testReport/ |
| Max. process+thread count | 3557 (vs. ulimit of 12500) |
| 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-2284/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-680667322
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 25s | 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 24s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 41s | master passed |
| +1 :green_heart: | compile | 1m 39s | master passed |
| +1 :green_heart: | shadedjars | 6m 35s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 17s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 23s | the patch passed |
| +1 :green_heart: | compile | 1m 41s | the patch passed |
| +1 :green_heart: | javac | 1m 41s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 30s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 15s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 2s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 26s | hbase-replication in the patch passed. |
| -1 :x: | unit | 144m 15s | hbase-server in the patch failed. |
| | | 175m 20s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d1ff878a481e 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 / 01cf60067c |
| Default Java | 1.8.0_232 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/2/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-2284/2/testReport/ |
| Max. process+thread count | 4837 (vs. ulimit of 12500) |
| 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-2284/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-682297344
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 28s | 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 24s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 29s | master passed |
| +1 :green_heart: | compile | 2m 8s | master passed |
| +1 :green_heart: | shadedjars | 7m 58s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 31s | hbase-client in master failed. |
| -0 :warning: | javadoc | 0m 17s | hbase-replication in master failed. |
| -0 :warning: | javadoc | 0m 43s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 4s | 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 | 6m 38s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 26s | hbase-client in the patch failed. |
| -0 :warning: | javadoc | 0m 16s | hbase-replication in the patch failed. |
| -0 :warning: | javadoc | 0m 40s | hbase-server in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 12s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 27s | hbase-replication in the patch passed. |
| +1 :green_heart: | unit | 130m 52s | hbase-server in the patch passed. |
| | | 167m 11s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux a6418bf2dba0 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 / 047e0618d2 |
| Default Java | 2020-01-14 |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-replication.txt |
| javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/5/testReport/ |
| Max. process+thread count | 4087 (vs. ulimit of 12500) |
| 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-2284/5/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#issuecomment-676801030
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 40s | 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 | 4m 46s | master passed |
| +1 :green_heart: | compile | 1m 57s | master passed |
| +1 :green_heart: | shadedjars | 7m 8s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 23s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 27s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 38s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 10s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 0m 27s | hbase-replication in the patch passed. |
| -1 :x: | unit | 151m 34s | hbase-server in the patch failed. |
| | | 186m 24s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2284 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 4b53bbfada3c 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 / 1164531d5a |
| Default Java | 1.8.0_232 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2284/1/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-2284/1/testReport/ |
| Max. process+thread count | 4419 (vs. ulimit of 12500) |
| 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-2284/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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 #2284: Hbase-24764: Add support of adding default peer configs via hbase-site.xml for all replication peers.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #2284:
URL: https://github.com/apache/hbase/pull/2284#discussion_r478608776
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -441,6 +442,41 @@ public void testCyclicReplication3() throws Exception {
}
}
+ /**
+ * Tests that base replication peer configs are applied on peer creation
+ * and the is overriden if updated as part of updateReplicationPeerConfig()
+ *
+ */
+ @Test
+ public void testBasePeerConfigsAppliedOnPeerCreationAndUpdate()
+ throws Exception {
+ LOG.info("testBasePeerConfigsAppliedOnPeerCreation");
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+ try {
+ baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+ startMiniClusters(2);
+ addPeer("1", 0, 1);
+ Admin admin = utilities[0].getAdmin();
+
+ Assert.assertEquals(customPeerConfigValue, admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfig.
+ newBuilder(admin.getReplicationPeerConfig("1")).
+ putConfiguration(customPeerConfigKey,customPeerConfigUpdatedValue).build();
+ admin.updateReplicationPeerConfig("1", updatedReplicationPeerConfig);
+
+ Assert.assertEquals(customPeerConfigUpdatedValue, admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+ }finally {
Review comment:
nit: } finally
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else null.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.
+ newBuilder(receivedPeerConfig);
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if (defaultPeerConfigs != null && defaultPeerConfigs.length() != 0) {
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
+
+ for (String defaultPeerConfig : defaultPeerConfigList) {
+ String[] configSplit = defaultPeerConfig.split("=");
+
+ if (configSplit != null && configSplit.length == 2) {
+ String configName = configSplit[0];
+ String configValue = configSplit[1];
+
+ // Only override if base config does not exist in existing peer configs
+ if (!peerConfigurations.containsKey(configName)) {
+ copiedPeerConfigBuilder.putConfiguration(configName,configValue);
+ isPeerConfigChanged = true;
+ }
+ }
+ }
+ }
+
+ return isPeerConfigChanged ? copiedPeerConfigBuilder.build() : null;
Review comment:
Why this? Just change the receivedPeerConfig in place?
##########
File path: hbase-replication/src/test/java/org/apache/hadoop/hbase/replication/TestZKReplicationPeerStorage.java
##########
@@ -215,4 +219,51 @@ public void testNoSyncReplicationState()
assertNotEquals(-1, ZKUtil.checkExists(UTIL.getZooKeeperWatcher(),
STORAGE.getNewSyncReplicationStateNode(peerId)));
}
+
+ @Test
+ public void testBaseReplicationPeerConfigIsAppliedIfNotAlreadySet(){
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+
+ String customPeerConfigSecondKey = "hbase.xxx.custom_second_config";
+ String customPeerConfigSecondValue = "testSecond";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = getConfig(1);
+
+ // custom config not present
+ assertEquals(existingReplicationPeerConfig.getConfiguration().get(customPeerConfigKey), null);
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue).concat(";").
+ concat(customPeerConfigSecondKey).concat("=").concat(customPeerConfigSecondValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
+
+ assertEquals(customPeerConfigValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigKey));
+ assertEquals(customPeerConfigSecondValue, updatedReplicationPeerConfig.getConfiguration().
+ get(customPeerConfigSecondKey));
+ }
+
+ @Test
+ public void testBaseReplicationPeerConfigDoesNotOverrideIfAlreadySet(){
+
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigOldValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+
+ ReplicationPeerConfig existingReplicationPeerConfig = ReplicationPeerConfig.
+ newBuilder(getConfig(1))
+ .putConfiguration(customPeerConfigKey,customPeerConfigOldValue).build();
+
+ Configuration conf = UTIL.getConfiguration();
+ conf.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigUpdatedValue));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfigUtil.
+ addBasePeerConfigsIfNotPresent(conf,existingReplicationPeerConfig);
Review comment:
I lost your comment in the force push, but I think we can merge both these tests into testReplicationBaseConfig() or some such... We cam just add these last two lines in the above test and we don't need all the other boilerplate.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +451,45 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ Sample Configuration
+ <property>
+ <name>hbase.replication.peer.default.configs</name>
+ <value>hbase.replication.source.custom.walentryfilters=x,y,z;hbase.xxx.custom_property=123</value>
+ </property>
+ */
+
+ /**
+ * Helper method to add default peer configs from HBase Configuration to ReplicationPeerConfig
+ * @param conf Configuration
+ * @return true if new configurations was added.
+ */
+ public static ReplicationPeerConfig addDefaultPeerConfigsIfNotPresent(Configuration conf, ReplicationPeerConfig receivedPeerConfig){
+
+ ReplicationPeerConfigBuilder copiedPeerConfigBuilder = ReplicationPeerConfig.newBuilder(receivedPeerConfig);
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_DEFAULT_CONFIG);
+
+ Map<String,String> peerConfigurations = receivedPeerConfig.getConfiguration();
+
+ if(defaultPeerConfigs != null && defaultPeerConfigs.length() != 0){
+ String[] defaultPeerConfigList = defaultPeerConfigs.split(";");
Review comment:
Looks like you missed this, this code can be condensed into
Splitter.on(';').withKeyValueSeparator('=').split(value).trimResults();
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else null.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
Review comment:
nit: space between ) {
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationPeerConfigUtil.java
##########
@@ -450,6 +452,49 @@ public static ReplicationPeerConfig appendTableCFsToReplicationPeerConfig(
return builder.build();
}
+ /**
+ * 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"
+ *
+ * @param conf Configuration
+ * @return ReplicationPeerConfig if peer configurations are updated else null.
+ */
+ public static ReplicationPeerConfig addBasePeerConfigsIfNotPresent(Configuration conf,
+ ReplicationPeerConfig receivedPeerConfig){
+ boolean isPeerConfigChanged = false;
+ String defaultPeerConfigs = conf.get(HBASE_REPLICATION_PEER_BASE_CONFIG,null);
Review comment:
nit: space between , null
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java
##########
@@ -441,6 +442,41 @@ public void testCyclicReplication3() throws Exception {
}
}
+ /**
+ * Tests that base replication peer configs are applied on peer creation
+ * and the is overriden if updated as part of updateReplicationPeerConfig()
+ *
+ */
+ @Test
+ public void testBasePeerConfigsAppliedOnPeerCreationAndUpdate()
+ throws Exception {
+ LOG.info("testBasePeerConfigsAppliedOnPeerCreation");
+ String customPeerConfigKey = "hbase.xxx.custom_config";
+ String customPeerConfigValue = "test";
+ String customPeerConfigUpdatedValue = "test_updated";
+ try {
+ baseConfiguration.set(ReplicationPeerConfigUtil.HBASE_REPLICATION_PEER_BASE_CONFIG,
+ customPeerConfigKey.concat("=").concat(customPeerConfigValue));
+ startMiniClusters(2);
+ addPeer("1", 0, 1);
+ Admin admin = utilities[0].getAdmin();
+
+ Assert.assertEquals(customPeerConfigValue, admin.getReplicationPeerConfig("1").
+ getConfiguration().get(customPeerConfigKey));
+
+ ReplicationPeerConfig updatedReplicationPeerConfig = ReplicationPeerConfig.
+ newBuilder(admin.getReplicationPeerConfig("1")).
+ putConfiguration(customPeerConfigKey,customPeerConfigUpdatedValue).build();
+ admin.updateReplicationPeerConfig("1", updatedReplicationPeerConfig);
Review comment:
Can you also add some tests for deleting a configuration? delete an override, base config should be picked up if it is present
Also, add another peer and make sure it only has the base configuration and not the updated values above.
Also, add a peer, update base configuration, restart cluster, new base should be picked up (our target usecase essentially).
----------------------------------------------------------------
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