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/01/14 18:09:00 UTC
[GitHub] [hbase] bharathv opened a new pull request #1039: HBASE-23647: Make
MasterRegistry the default impl.
bharathv opened a new pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369933775
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
This code is going waste away. If user chooses zk registry, this code applies?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370881940
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/security/token/SecureTestCluster.java
##########
@@ -80,8 +80,8 @@ public static void setUp() throws Exception {
public static void tearDown() throws Exception {
if (CLUSTER != null) {
CLUSTER.shutdown();
+ CLUSTER.join();
}
- CLUSTER.join();
Review comment:
🤦♀
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-577797028
> I suppose we have same issue w/ zk ensemble list.
Ya, thats the same problem, agreed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370961282
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAlwaysStandByHMaster.java
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.StartMiniClusterOption;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category({MediumTests.class, MasterTests.class})
+public class TestAlwaysStandByHMaster {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestAlwaysStandByHMaster.class);
+ private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
+
+ @BeforeClass
+ public static void setup() throws Exception {
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369995231
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/AlwaysStandByMasterManager.java
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.Server;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.monitoring.MonitoredTask;
+import org.apache.hadoop.hbase.util.Threads;
+import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An implementation of ActiveMasterManager that never transitions it's master to active state. It
+ * always remains as a stand by master. With the master registry implementation (HBASE-18095) it is
+ * expected to have at least one active / standby master always running at any point in time since
+ * they serve as the gateway for client connections.
+ *
+ * With this implementation, tests can simulate the scenario of not having an active master yet the
+ * client connections to the cluster succeed.
Review comment:
ya the regular RetryingRpcCaller with long stack traces :-)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-576253994
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 19s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 40 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 39s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 42s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 44s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 34s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 57s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 19s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 56s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 48s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 32s | the patch passed |
| +1 :green_heart: | compile | 1m 44s | the patch passed |
| +1 :green_heart: | javac | 1m 44s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 25s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 35s | The patch passed checkstyle in hbase-client |
| -1 :x: | checkstyle | 1m 42s | hbase-server: The patch generated 9 new + 450 unchanged - 2 fixed = 459 total (was 452) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 3s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 13s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 16s | the patch passed |
| +1 :green_heart: | findbugs | 7m 16s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 20s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 1m 59s | hbase-client in the patch passed. |
| -1 :x: | unit | 200m 42s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 32s | The patch does not generate ASF License warnings. |
| | | 281m 4s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.replication.multiwal.TestReplicationSyncUpToolWithMultipleAsyncWAL |
| | hadoop.hbase.replication.TestReplicationSyncUpTool |
| | hadoop.hbase.master.TestMasterShutdown |
| | hadoop.hbase.master.assignment.TestRegionMoveAndAbandon |
| | hadoop.hbase.replication.multiwal.TestReplicationSyncUpToolWithMultipleWAL |
| | hadoop.hbase.master.assignment.TestRegionSplit |
| | hadoop.hbase.replication.TestReplicationDroppedTables |
| | hadoop.hbase.replication.TestReplicationDisableInactivePeer |
| | hadoop.hbase.client.TestFromClientSide |
| | hadoop.hbase.replication.TestReplicationSyncUpToolWithBulkLoadedData |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 14e3f113aa0b 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/3/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/3/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/3/testReport/ |
| Max. process+thread count | 5659 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/3/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369984248
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -170,27 +171,36 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
this.masterClass = (Class<? extends HMaster>)
conf.getClass(HConstants.MASTER_IMPL, masterClass);
// Start the HMasters.
- for (int i = 0; i < noMasters; i++) {
+ int i;
+ for (i = 0; i < noMasters; i++) {
addMaster(new Configuration(conf), i);
}
-
- // Populate the master address host ports in the config. This is needed if a master based
- // registry is configured for client metadata services (HBASE-18095)
- List<String> masterHostPorts = new ArrayList<>();
- getMasters().forEach(masterThread ->
- masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
- conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));
-
+ for (int j = 0; j < noAlwaysStandByMasters; j++) {
+ Configuration c = new Configuration(conf);
+ c.set(HConstants.MASTER_IMPL, "org.apache.hadoop.hbase.master.AlwaysStandByHMaster");
+ addMaster(c, i + j);
+ }
// Start the HRegionServers.
this.regionServerClass =
(Class<? extends HRegionServer>)conf.getClass(HConstants.REGION_SERVER_IMPL,
regionServerClass);
- for (int i = 0; i < noRegionServers; i++) {
- addRegionServer(new Configuration(conf), i);
+ for (int j = 0; j < noRegionServers; j++) {
+ addRegionServer(new Configuration(conf), j);
}
}
+ /**
+ * Populates the master address host ports in the config. This is needed if a master based
+ * registry is configured for client metadata services (HBASE-18095)
+ */
+ private void refreshMasterAddrsConfig() {
+ List<String> masterHostPorts = new ArrayList<>();
+ getMasters().forEach(masterThread ->
+ masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
+ conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));
Review comment:
Ah I rewrote this multiple times to get it working and in that process called it from multiple places, hence separated it out into a method. Doesn't make sense anymore like you pointed out. Moved it to the new place instead.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369992690
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithCoprocessor.java
##########
@@ -43,7 +43,7 @@
@Parameterized.Parameters
public static Collection parameters() {
return Arrays.asList(new Object[][] {
- { ZKConnectionRegistry.class, 1}
+ { MasterRegistry.class, 1}
Review comment:
This test is a bit special, I had to parameterize it in the first place to override parent test params. The parent test (TestFromClientSide) has a lot of coverage for ZKConnectionRegistry and I felt this is redundant (tests the same thing) so removed. You prefer to retain it?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369991092
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/StartMiniClusterOption.java
##########
@@ -46,6 +46,14 @@
* can find the active/primary master with {@link MiniHBaseCluster#getMaster()}.
*/
private final int numMasters;
+
+ /**
+ * Number of masters that always remain standby. These set of masters never transition to active
+ * even if an active master does not exist. These are needed for testing scenarios where there are
+ * no active masters in the cluster but the cluster connection (backed by master registry) should
+ * still work.
+ */
+ private final int numAlwaysStandByMasters;
Review comment:
Agree, it is a test-only construct. Didn't leak into the server module. Anything else should be done?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-578365690
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 8s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 47 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 52s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 47s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 17s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 17s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 47s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 40s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 35s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 22s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 30s | The patch passed checkstyle in hbase-client |
| +1 :green_heart: | checkstyle | 1m 22s | hbase-server: The patch generated 0 new + 443 unchanged - 3 fixed = 443 total (was 446) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 11s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 31s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 17s | the patch passed |
| +1 :green_heart: | findbugs | 6m 45s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 9s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 1m 58s | hbase-client in the patch passed. |
| -1 :x: | unit | 180m 53s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 44s | The patch does not generate ASF License warnings. |
| | | 260m 33s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.security.provider.TestCustomSaslAuthenticationProvider |
| | hadoop.hbase.client.TestFromClientSide |
| | hadoop.hbase.regionserver.TestRegionReplicasWithRestartScenarios |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/8/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 76dc38feb558 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / 62da419b23 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/8/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/8/testReport/ |
| Max. process+thread count | 5289 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/8/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-577680324
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 2s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 47 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 36s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 11s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 46s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 30s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 37s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 22s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 30s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 22s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ 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 | 1m 45s | the patch passed |
| +1 :green_heart: | javac | 1m 45s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 24s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 32s | The patch passed checkstyle in hbase-client |
| +1 :green_heart: | checkstyle | 1m 30s | hbase-server: The patch generated 0 new + 450 unchanged - 3 fixed = 450 total (was 453) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 41s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 15m 28s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 20s | the patch passed |
| +1 :green_heart: | findbugs | 6m 48s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 21s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 2m 4s | hbase-client in the patch passed. |
| -1 :x: | unit | 160m 41s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 40s | The patch does not generate ASF License warnings. |
| | | 235m 22s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.io.hfile.bucket.TestBucketCache |
| | hadoop.hbase.client.TestFromClientSide |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/7/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 52f07988c23c 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/7/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/7/testReport/ |
| Max. process+thread count | 5730 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/7/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369994908
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaRegionLocationCache.java
##########
@@ -60,7 +60,7 @@ public static void setUp() throws Exception {
TEST_UTIL.getConfiguration().setInt(HConstants.META_REPLICAS_NUM, 3);
TEST_UTIL.startMiniCluster(3);
REGISTRY = ConnectionRegistryFactory.getRegistry(TEST_UTIL.getConfiguration());
- RegionReplicaTestHelper.waitUntilAllMetaReplicasHavingRegionLocation(
+ RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(TEST_UTIL,
Review comment:
good catch, you are right. For some reason I thought there are some callers that pass some custom config but I was wrong.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370960558
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -134,9 +140,9 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
*/
@SuppressWarnings("unchecked")
public LocalHBaseCluster(final Configuration conf, final int noMasters,
- final int noRegionServers, final Class<? extends HMaster> masterClass,
- final Class<? extends HRegionServer> regionServerClass)
- throws IOException {
+ final int noAlwaysStandByMasters, final int noRegionServers,
Review comment:
Ya, too long and confusing now.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369941187
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
##########
@@ -3058,6 +3061,26 @@ private void initConnection() throws IOException {
this.asyncConnection = ClusterConnectionFactory.createAsyncClusterConnection(conf, null, user);
}
+ /**
+ * Resets the connections so that the next time getConnection() is called, a new connection is
+ * created. This is needed in cases where the entire cluster / all the masters are shutdown and
+ * the connection is not valid anymore.
+ * TODO: There should be a more coherent way of doing this. Unfortunately the way tests are
+ * written, not all start() stop() calls go through this class. Most tests directly operate on
+ * the underlying mini/local hbase cluster. That makes it difficult for this wrapper class to
+ * maintain the connection state automatically. Cleaning this is a much bigger refactor.
Review comment:
Good comment
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370879179
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithCoprocessor.java
##########
@@ -43,7 +43,7 @@
@Parameterized.Parameters
public static Collection parameters() {
return Arrays.asList(new Object[][] {
- { ZKConnectionRegistry.class, 1}
+ { MasterRegistry.class, 1}
Review comment:
I'd prefer to retain both. Let's follow on with some work to establish code coverage and see about pruning the tests via that angle.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r371497725
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
> Overall, I think all MR jobs running on a single cluster will use master registry. I think that answers the MR/Spark/Flink usecases.
Sounds good.
> I think these need to be rewritten with new config params like hbase.[source|target].master.addrs for clients to pass the addresses so that they can use master registry.
I think it's best to reduce the ZK-exposed surface area as much as possible. This seems a reasonable solution to me.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369943132
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/StartMiniClusterOption.java
##########
@@ -46,6 +46,14 @@
* can find the active/primary master with {@link MiniHBaseCluster#getMaster()}.
*/
private final int numMasters;
+
+ /**
+ * Number of masters that always remain standby. These set of masters never transition to active
+ * even if an active master does not exist. These are needed for testing scenarios where there are
+ * no active masters in the cluster but the cluster connection (backed by master registry) should
+ * still work.
+ */
+ private final int numAlwaysStandByMasters;
Review comment:
Oh, so a hack to get around some test scenarios. Ok. Good.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369939988
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -170,27 +171,36 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
this.masterClass = (Class<? extends HMaster>)
conf.getClass(HConstants.MASTER_IMPL, masterClass);
// Start the HMasters.
- for (int i = 0; i < noMasters; i++) {
+ int i;
+ for (i = 0; i < noMasters; i++) {
addMaster(new Configuration(conf), i);
}
-
- // Populate the master address host ports in the config. This is needed if a master based
- // registry is configured for client metadata services (HBASE-18095)
- List<String> masterHostPorts = new ArrayList<>();
- getMasters().forEach(masterThread ->
- masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
- conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));
-
+ for (int j = 0; j < noAlwaysStandByMasters; j++) {
+ Configuration c = new Configuration(conf);
+ c.set(HConstants.MASTER_IMPL, "org.apache.hadoop.hbase.master.AlwaysStandByHMaster");
+ addMaster(c, i + j);
+ }
// Start the HRegionServers.
this.regionServerClass =
(Class<? extends HRegionServer>)conf.getClass(HConstants.REGION_SERVER_IMPL,
regionServerClass);
- for (int i = 0; i < noRegionServers; i++) {
- addRegionServer(new Configuration(conf), i);
+ for (int j = 0; j < noRegionServers; j++) {
+ addRegionServer(new Configuration(conf), j);
}
}
+ /**
+ * Populates the master address host ports in the config. This is needed if a master based
+ * registry is configured for client metadata services (HBASE-18095)
+ */
+ private void refreshMasterAddrsConfig() {
+ List<String> masterHostPorts = new ArrayList<>();
+ getMasters().forEach(masterThread ->
+ masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
+ conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));
Review comment:
I could be obnoxious here but won't. Better to do as you have done here and just move the code.... not change it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-575645245
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 5 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 35s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 48s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 46s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 30s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 5s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 18s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 5m 3s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 57s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 40s | the patch passed |
| +1 :green_heart: | compile | 1m 45s | the patch passed |
| +1 :green_heart: | javac | 1m 45s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 26s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 34s | The patch passed checkstyle in hbase-client |
| -1 :x: | checkstyle | 1m 28s | hbase-server: The patch generated 2 new + 32 unchanged - 1 fixed = 34 total (was 33) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 2s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 20s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 15s | the patch passed |
| +1 :green_heart: | findbugs | 7m 12s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 9s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 1m 55s | hbase-client in the patch passed. |
| -1 :x: | unit | 220m 38s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 45s | The patch does not generate ASF License warnings. |
| | | 301m 15s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.client.TestAsyncMetaRegionLocator |
| | hadoop.hbase.replication.multiwal.TestReplicationSyncUpToolWithMultipleAsyncWAL |
| | hadoop.hbase.client.TestReplicasClient |
| | hadoop.hbase.security.token.TestGenerateDelegationToken |
| | hadoop.hbase.replication.TestReplicationStatusSourceStartedTargetStoppedNewOp |
| | hadoop.hbase.ipc.TestRpcClientLeaks |
| | hadoop.hbase.regionserver.TestRegionServerNoMaster |
| | hadoop.hbase.master.TestMasterNoCluster |
| | hadoop.hbase.replication.TestReplicationStatusSourceStartedTargetStoppedWithRecovery |
| | hadoop.hbase.replication.TestReplicationSyncUpTool |
| | hadoop.hbase.regionserver.TestClusterId |
| | hadoop.hbase.replication.TestReplicationStatusBothNormalAndRecoveryLagging |
| | hadoop.hbase.master.TestMigrateNamespaceTable |
| | hadoop.hbase.client.TestMetaWithReplicas |
| | hadoop.hbase.replication.TestReplicationStatusAfterLagging |
| | hadoop.hbase.security.token.TestTokenAuthentication |
| | hadoop.hbase.replication.regionserver.TestRegionReplicaReplicationEndpointNoMaster |
| | hadoop.hbase.replication.multiwal.TestReplicationSyncUpToolWithMultipleWAL |
| | hadoop.hbase.master.assignment.TestRegionSplit |
| | hadoop.hbase.replication.TestReplicationDroppedTables |
| | hadoop.hbase.namespace.TestNamespaceAuditor |
| | hadoop.hbase.master.TestMasterOperationsForRegionReplicas |
| | hadoop.hbase.replication.TestReplicationDisableInactivePeer |
| | hadoop.hbase.client.TestConnection |
| | hadoop.hbase.client.TestFromClientSide |
| | hadoop.hbase.replication.TestReplicationStatusSourceStartedTargetStoppedNoOps |
| | hadoop.hbase.security.token.TestDelegationTokenWithEncryption |
| | hadoop.hbase.TestClusterBootOrder |
| | hadoop.hbase.replication.TestReplicationSyncUpToolWithBulkLoadedData |
| | hadoop.hbase.regionserver.TestSplitTransactionOnCluster |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux badb4f70380b 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/2/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/2/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/2/testReport/ |
| Max. process+thread count | 4812 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/2/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370873810
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
I'm finding this method used by `SyncTable`, `TableMapReduceUtil`, `TableOutputFormat`, `VerifyReplication`, `ExportSnapshot` ... Per earlier discussion, replication will continue to use the ZK registry... what about the general MapReduce/Spark/Flink use-cases?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369944209
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithCoprocessor.java
##########
@@ -43,7 +43,7 @@
@Parameterized.Parameters
public static Collection parameters() {
return Arrays.asList(new Object[][] {
- { ZKConnectionRegistry.class, 1}
+ { MasterRegistry.class, 1}
Review comment:
What you thinking? No tests of old registry in Master?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369991449
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncAdminMasterSwitch.java
##########
@@ -48,8 +48,6 @@ public void testSwitch() throws IOException, InterruptedException {
assertEquals(TEST_UTIL.getHBaseCluster().getRegionServerThreads().size(),
admin.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.SERVERS_NAME)).join()
.getServersName().size());
- // stop the old master, and start a new one
- TEST_UTIL.getMiniHBaseCluster().startMaster();
Review comment:
This extends TestAsyncAdminBase (file above) that builds the cluster. Fixed it there.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369996228
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java
##########
@@ -133,11 +134,19 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception {
util.startMiniZKCluster();
util.createRootDir();
final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
+ new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class,
MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
final int MASTER_INDEX = 0;
final MasterThread master = cluster.getMasters().get(MASTER_INDEX);
master.start();
+ // Switching to master registry exposed a race in the master bootstrap that can result in a
+ // lost shutdown command (HBASE-8422). The race is essentially because the server manager in
+ // HMaster is not initialized by the time shutdown() RPC (below) is made to
+ // the master. The reason it was not happening earlier is because the connection creation with
+ // ZK registry is so slow that by then the server manager is init'ed thus masking the problem.
+ // For now, I'm putting a wait() here to workaround the issue, I think the fix for it is a
+ // little delicate and needs to be done separately.
+ Waiter.waitFor(conf, 5000, () -> master.getMaster().getServerManager() != null);
Review comment:
> BTW, does test suite run faster now?
So the initial impression is that the individual tests seem to run a little faster (in my local runs, anywhere a connection is involved), but I don't have actual numbers from the Jenkins jobs since the PR jenkins run doesn't always run the full suite. Once this patch makes it to the branch, we will some numbers to compare.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369894052
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,19 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. For example, if all the masters are down, region
+ // servers can abort at the same time, because the internal connection is master dependent and
+ // fails.This is an operational nightmare. Using the ZK based registry means that the region
+ // servers are now on the look out for new masters, if they are spun up.
Review comment:
Clarified.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370898120
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/AlwaysStandByMasterManager.java
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.Server;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.monitoring.MonitoredTask;
+import org.apache.hadoop.hbase.util.Threads;
+import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An implementation of ActiveMasterManager that never transitions it's master to active state. It
+ * always remains as a stand by master. With the master registry implementation (HBASE-18095) it is
+ * expected to have at least one active / standby master always running at any point in time since
+ * they serve as the gateway for client connections.
+ *
+ * With this implementation, tests can simulate the scenario of not having an active master yet the
+ * client connections to the cluster succeed.
+ */
+@InterfaceAudience.Private
+class AlwaysStandByMasterManager extends ActiveMasterManager {
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-574424141
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 45s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 54s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 7m 34s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 42s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 24s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 43s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 6s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 5m 37s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 58s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 7m 0s | the patch passed |
| +1 :green_heart: | compile | 1m 48s | the patch passed |
| +1 :green_heart: | javac | 1m 48s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 25s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 6m 2s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 23m 14s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 11s | the patch passed |
| +1 :green_heart: | findbugs | 7m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 57s | hbase-client in the patch passed. |
| -1 :x: | unit | 232m 7s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 5s | The patch does not generate ASF License warnings. |
| | | 323m 27s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.security.access.TestWithDisabledAuthorization |
| | hadoop.hbase.client.TestAsyncMetaRegionLocator |
| | hadoop.hbase.client.TestReplicaWithCluster |
| | hadoop.hbase.replication.multiwal.TestReplicationSyncUpToolWithMultipleAsyncWAL |
| | hadoop.hbase.client.TestReplicasClient |
| | hadoop.hbase.client.TestAsyncAdminMasterSwitch |
| | hadoop.hbase.client.TestFromClientSideWithCoprocessor |
| | hadoop.hbase.security.token.TestGenerateDelegationToken |
| | hadoop.hbase.replication.TestReplicationStatusSourceStartedTargetStoppedNewOp |
| | hadoop.hbase.ipc.TestRpcClientLeaks |
| | hadoop.hbase.regionserver.TestRegionServerNoMaster |
| | hadoop.hbase.master.TestMasterNoCluster |
| | hadoop.hbase.replication.TestReplicationStatusSourceStartedTargetStoppedWithRecovery |
| | hadoop.hbase.replication.TestReplicationSyncUpTool |
| | hadoop.hbase.regionserver.TestClusterId |
| | hadoop.hbase.TestHBaseTestingUtility |
| | hadoop.hbase.replication.TestReplicationStatusBothNormalAndRecoveryLagging |
| | hadoop.hbase.master.TestRollingRestart |
| | hadoop.hbase.master.TestMasterShutdown |
| | hadoop.hbase.master.TestMigrateNamespaceTable |
| | hadoop.hbase.security.access.TestAccessController |
| | hadoop.hbase.client.TestMetaWithReplicas |
| | hadoop.hbase.replication.TestReplicationStatusAfterLagging |
| | hadoop.hbase.security.token.TestTokenAuthentication |
| | hadoop.hbase.replication.regionserver.TestRegionReplicaReplicationEndpointNoMaster |
| | hadoop.hbase.replication.multiwal.TestReplicationSyncUpToolWithMultipleWAL |
| | hadoop.hbase.master.assignment.TestRegionSplit |
| | hadoop.hbase.replication.TestReplicationDroppedTables |
| | hadoop.hbase.security.access.TestSnapshotScannerHDFSAclController |
| | hadoop.hbase.namespace.TestNamespaceAuditor |
| | hadoop.hbase.master.TestMasterOperationsForRegionReplicas |
| | hadoop.hbase.replication.TestReplicationDisableInactivePeer |
| | hadoop.hbase.client.TestConnection |
| | hadoop.hbase.client.TestFromClientSide |
| | hadoop.hbase.replication.TestReplicationStatusSourceStartedTargetStoppedNoOps |
| | hadoop.hbase.security.token.TestDelegationTokenWithEncryption |
| | hadoop.hbase.client.TestAsyncReplicationAdminApiWithClusters |
| | hadoop.hbase.TestClusterBootOrder |
| | hadoop.hbase.client.TestClientTimeouts |
| | hadoop.hbase.replication.TestReplicationSyncUpToolWithBulkLoadedData |
| | hadoop.hbase.regionserver.TestSplitTransactionOnCluster |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux c7f1ede18924 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/1/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/1/testReport/ |
| Max. process+thread count | 5027 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/1/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
apurtell commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-577412722
So the only test failure now is TestFromClientSide? Can this be excluded as not related or a flake?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370898329
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionMoveAndAbandon.java
##########
@@ -116,6 +116,7 @@ public void test() throws Exception {
// Start up everything again
LOG.info("Starting cluster");
UTIL.getMiniHBaseCluster().startMaster();
+ UTIL.invalidateConnection();
Review comment:
ya, its bad. All the tests operate on the cluster object directly and changes its state. There is no way to keep track of restarts.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370960622
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSideWithCoprocessor.java
##########
@@ -43,7 +43,7 @@
@Parameterized.Parameters
public static Collection parameters() {
return Arrays.asList(new Object[][] {
- { ZKConnectionRegistry.class, 1}
+ { MasterRegistry.class, 1}
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369944879
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/AlwaysStandByMasterManager.java
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.Server;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.monitoring.MonitoredTask;
+import org.apache.hadoop.hbase.util.Threads;
+import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An implementation of ActiveMasterManager that never transitions it's master to active state. It
+ * always remains as a stand by master. With the master registry implementation (HBASE-18095) it is
+ * expected to have at least one active / standby master always running at any point in time since
+ * they serve as the gateway for client connections.
+ *
+ * With this implementation, tests can simulate the scenario of not having an active master yet the
+ * client connections to the cluster succeed.
Review comment:
Is it going to be hell when no Master for clients up in the cluster?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370878425
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/RegionReplicaTestHelper.java
##########
@@ -43,24 +42,30 @@ private RegionReplicaTestHelper() {
}
// waits for all replicas to have region location
- static void waitUntilAllMetaReplicasHavingRegionLocation(Configuration conf,
- ConnectionRegistry registry, int regionReplication) throws IOException {
+ static void waitUntilAllMetaReplicasAreReady(HBaseTestingUtility util,
+ ConnectionRegistry registry, int regionReplication) {
Review comment:
I know this is not your code, but, is that last argument `int regionReplication` necessary? Seems like there's already configuration available via the other arguments.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370871658
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
Review comment:
Is there a check-then-fail that can be done to assert that the method invocation has significance? Maybe log a warning saying zk configs are being applied to a master registry?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-574441500
A bunch of tests failed with "Connection Refused" errors on the master ports serving the RPCs. Let me dig into the logs and update here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370881472
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRegionMoveAndAbandon.java
##########
@@ -116,6 +116,7 @@ public void test() throws Exception {
// Start up everything again
LOG.info("Starting cluster");
UTIL.getMiniHBaseCluster().startMaster();
+ UTIL.invalidateConnection();
Review comment:
It's a pity this method must be called manually. Seems easy to forget.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369863370
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java
##########
@@ -133,11 +133,19 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception {
util.startMiniZKCluster();
util.createRootDir();
final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
+ new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class,
MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
final int MASTER_INDEX = 0;
final MasterThread master = cluster.getMasters().get(MASTER_INDEX);
master.start();
+ // Switching to master registry exposed a race in the master bootstrap that can result in a
+ // lost shutdown command (essentially HBASE-8422). The race is essentially because the
+ // server manager in HMaster is not initialized by the time shutdown() RPC (below) is made to
+ // the master. The reason it was not happening earlier is because the connection creation with
+ // ZK registry is so slow that by then the server manager is init'ed thus masking the problem.
+ // For now, I'm putting a sleep here to workaround the issue, I think the fix for it is a little
+ // delicate and needs to be done separately.
+ Thread.sleep(5000);
Review comment:
Is it possible to use Waiter#waitFor to wait on some condition variable or predicate? Hard coded sleeps in tests tend to fall over on Apache Jenkins because of load issues ie the sleeps can never be long enough...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369982726
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
Agree. It is ok to leave as-is since it is a no-op?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369945155
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java
##########
@@ -133,11 +134,19 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception {
util.startMiniZKCluster();
util.createRootDir();
final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
+ new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class,
MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
final int MASTER_INDEX = 0;
final MasterThread master = cluster.getMasters().get(MASTER_INDEX);
master.start();
+ // Switching to master registry exposed a race in the master bootstrap that can result in a
+ // lost shutdown command (HBASE-8422). The race is essentially because the server manager in
+ // HMaster is not initialized by the time shutdown() RPC (below) is made to
+ // the master. The reason it was not happening earlier is because the connection creation with
+ // ZK registry is so slow that by then the server manager is init'ed thus masking the problem.
+ // For now, I'm putting a wait() here to workaround the issue, I think the fix for it is a
+ // little delicate and needs to be done separately.
+ Waiter.waitFor(conf, 5000, () -> master.getMaster().getServerManager() != null);
Review comment:
Good
BTW, does test suite run faster now?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-576608535
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 47 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 36s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 7s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 47s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 30s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 36s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 22s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 40s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 31s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ 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 | 1m 44s | the patch passed |
| +1 :green_heart: | javac | 1m 44s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 26s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 32s | The patch passed checkstyle in hbase-client |
| +1 :green_heart: | checkstyle | 1m 31s | hbase-server: The patch generated 0 new + 451 unchanged - 2 fixed = 451 total (was 453) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 38s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 15m 42s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 21s | the patch passed |
| +1 :green_heart: | findbugs | 6m 46s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 20s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 2m 4s | hbase-client in the patch passed. |
| -1 :x: | unit | 160m 26s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 38s | The patch does not generate ASF License warnings. |
| | | 235m 31s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.client.TestFromClientSide |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux da50f0173692 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/5/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/5/testReport/ |
| Max. process+thread count | 5424 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/5/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370879778
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/AlwaysStandByMasterManager.java
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.Server;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.monitoring.MonitoredTask;
+import org.apache.hadoop.hbase.util.Threads;
+import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An implementation of ActiveMasterManager that never transitions it's master to active state. It
+ * always remains as a stand by master. With the master registry implementation (HBASE-18095) it is
+ * expected to have at least one active / standby master always running at any point in time since
+ * they serve as the gateway for client connections.
+ *
+ * With this implementation, tests can simulate the scenario of not having an active master yet the
+ * client connections to the cluster succeed.
+ */
+@InterfaceAudience.Private
+class AlwaysStandByMasterManager extends ActiveMasterManager {
Review comment:
nit: could be static inner class of `AlwaysStandByHMaster`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369945016
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAlwaysStandByHMaster.java
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.StartMiniClusterOption;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category({MediumTests.class, MasterTests.class})
+public class TestAlwaysStandByHMaster {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestAlwaysStandByHMaster.class);
+ private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
+
+ @BeforeClass
+ public static void setup() throws Exception {
+ StartMiniClusterOption option = StartMiniClusterOption.builder().
+ numAlwaysStandByMasters(1).numMasters(1).numRegionServers(1).build();
+ TEST_UTIL.startMiniCluster(option);
+ }
+
+ public static void teardown() throws Exception {
+ TEST_UTIL.shutdownMiniCluster();
+ }
+
+ /**
+ * Tests that the AlwaysStandByHMaster does not transition to active state even if no active
+ * master exists.
+ */
+ @Test public void testAlwaysStandBy() throws Exception {
+ // Make sure there is an active master.
+ assertNotNull(TEST_UTIL.getMiniHBaseCluster().getMaster());
+ assertEquals(2, TEST_UTIL.getMiniHBaseCluster().getMasterThreads().size());
+ // Kill the only active master.
+ TEST_UTIL.getMiniHBaseCluster().stopMaster(0).join();
+ // Wait for 5s to make sure the always standby doesn't transition to active state.
+ assertFalse(TEST_UTIL.getMiniHBaseCluster().waitForActiveAndReadyMaster(5000));
+ // Add a new master.
+ HMaster newActive = TEST_UTIL.getMiniHBaseCluster().startMaster().getMaster();
+ assertTrue(TEST_UTIL.getMiniHBaseCluster().waitForActiveAndReadyMaster(5000));
+ // Newly added master should be the active.
+ assertEquals(newActive.getServerName(),
+ TEST_UTIL.getMiniHBaseCluster().getMaster().getServerName());
+ }
+}
Review comment:
Good
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-576514772
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 5s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 47 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 33s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 45s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 46s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 37s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 58s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 17s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 5m 8s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 7m 3s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 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: | checkstyle | 0m 25s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 33s | The patch passed checkstyle in hbase-client |
| -1 :x: | checkstyle | 1m 38s | hbase-server: The patch generated 1 new + 451 unchanged - 2 fixed = 452 total (was 453) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 1s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 22s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 17s | the patch passed |
| +1 :green_heart: | findbugs | 7m 17s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 8s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 1m 55s | hbase-client in the patch passed. |
| -1 :x: | unit | 168m 33s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 12s | The patch does not generate ASF License warnings. |
| | | 248m 20s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.quotas.TestClusterScopeQuotaThrottle |
| | hadoop.hbase.client.TestFromClientSide |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 4816f481e385 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/4/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/4/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/4/testReport/ |
| Max. process+thread count | 5450 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/4/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370874480
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -134,9 +140,9 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
*/
@SuppressWarnings("unchecked")
public LocalHBaseCluster(final Configuration conf, final int noMasters,
- final int noRegionServers, final Class<? extends HMaster> masterClass,
- final Class<? extends HRegionServer> regionServerClass)
- throws IOException {
+ final int noAlwaysStandByMasters, final int noRegionServers,
Review comment:
Ugh. The final constructor args are `Configuration, int, int, int, Class, Class`. It's time for a builder with named arguments.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369894075
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,19 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. For example, if all the masters are down, region
+ // servers can abort at the same time, because the internal connection is master dependent and
+ // fails.This is an operational nightmare. Using the ZK based registry means that the region
+ // servers are now on the look out for new masters, if they are spun up.
+ // - Configuration management for region servers (cluster internal) is much simpler when adding
+ // new masters etc.
+ // - We need to retain ZKConnectionRegistry for replication use anyway, so we just extend it for
+ // other internal connections too.
+ conf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
+ "org.apache.hadoop.hbase.client.ZKConnectionRegistry");
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369983538
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -170,27 +171,36 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
this.masterClass = (Class<? extends HMaster>)
conf.getClass(HConstants.MASTER_IMPL, masterClass);
// Start the HMasters.
- for (int i = 0; i < noMasters; i++) {
+ int i;
+ for (i = 0; i < noMasters; i++) {
addMaster(new Configuration(conf), i);
}
-
- // Populate the master address host ports in the config. This is needed if a master based
- // registry is configured for client metadata services (HBASE-18095)
- List<String> masterHostPorts = new ArrayList<>();
- getMasters().forEach(masterThread ->
- masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
- conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));
-
+ for (int j = 0; j < noAlwaysStandByMasters; j++) {
+ Configuration c = new Configuration(conf);
+ c.set(HConstants.MASTER_IMPL, "org.apache.hadoop.hbase.master.AlwaysStandByHMaster");
Review comment:
Looks like you already figured it out based on your other comments below.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-578468772
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 20s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 48 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 45s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 47s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 15s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 7s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 18s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 45s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 41s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 38s | the patch passed |
| +1 :green_heart: | compile | 1m 47s | the patch passed |
| +1 :green_heart: | javac | 1m 47s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 22s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 31s | The patch passed checkstyle in hbase-client |
| +1 :green_heart: | checkstyle | 1m 22s | hbase-server: The patch generated 0 new + 443 unchanged - 3 fixed = 443 total (was 446) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 10s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 34s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 18s | the patch passed |
| +1 :green_heart: | findbugs | 7m 9s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 10s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 1m 59s | hbase-client in the patch passed. |
| -1 :x: | unit | 166m 30s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 21s | The patch does not generate ASF License warnings. |
| | | 246m 39s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.client.TestFromClientSideWithCoprocessor |
| | hadoop.hbase.security.provider.TestCustomSaslAuthenticationProvider |
| | hadoop.hbase.client.TestFromClientSide |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/9/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 13716f3ff67c 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / 39ff6f82d9 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/9/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/9/testReport/ |
| Max. process+thread count | 5470 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/9/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369861495
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
You could use findClass and if there's an exception fall through to alternate or recovery code. Anyway, agreed, a reference to a class constant is not called for here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370881236
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java
##########
@@ -132,12 +133,19 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception {
util.startMiniDFSCluster(3);
util.startMiniZKCluster();
util.createRootDir();
- final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
- MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
+ final LocalHBaseCluster cluster = new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS,
+ HMaster.class, MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
final int MASTER_INDEX = 0;
final MasterThread master = cluster.getMasters().get(MASTER_INDEX);
master.start();
+ // Switching to master registry exposed a race in the master bootstrap that can result in a
+ // lost shutdown command (HBASE-8422). The race is essentially because the server manager in
+ // HMaster is not initialized by the time shutdown() RPC (below) is made to
+ // the master. The reason it was not happening earlier is because the connection creation with
+ // ZK registry is so slow that by then the server manager is init'ed thus masking the problem.
+ // For now, I'm putting a wait() here to workaround the issue, I think the fix for it is a
+ // little delicate and needs to be done separately.
Review comment:
What's the fix you have in mind? Reading through your comment, I thought of a test-only coprocessor that installs a latch that can be waited upon by the test.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369990461
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,17 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. RegionServers can continue be up independent of
+ // masters' availability.
+ // - Configuration management for region servers (cluster internal) is much simpler when adding
+ // new masters etc.
Review comment:
(clarified it in the comment, let me know if it is still unclear)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370965852
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
Hmm. Good point.
TableMapReduceUtil: This happens only in initCredentialsForCluster() or if hbase.mapred.output.quorum is specified. Basically it only happens for a "peer" cluster. (same for TableOutputFormat, unless QUORUM_ADDRESS for a target is specified this doesn't happen). Overall, I think all MR jobs running on a single cluster will use master registry. I think that answers the MR/Spark/Flink usecases.
Now coming to MR jobs spanning multiple clusters (source and target) Ex: VerifyReplication / ExportSnapshot/SyncTable/TableOutputFormat etc
I think these need to be rewritten with new config params like hbase.[source|target].master.addrs for clients to pass the addresses so that they can use master registry.
What do you think? Should we rewrite them with new configs or maintain compatibility and keep using zkregistry?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369862135
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,19 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. For example, if all the masters are down, region
+ // servers can abort at the same time, because the internal connection is master dependent and
+ // fails.This is an operational nightmare. Using the ZK based registry means that the region
+ // servers are now on the look out for new masters, if they are spun up.
Review comment:
The tense here is confusing.
"Using the ZK based registry means the region servers continue to be independent of master availability..." ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369944404
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaRegionLocationCache.java
##########
@@ -60,7 +60,7 @@ public static void setUp() throws Exception {
TEST_UTIL.getConfiguration().setInt(HConstants.META_REPLICAS_NUM, 3);
TEST_UTIL.startMiniCluster(3);
REGISTRY = ConnectionRegistryFactory.getRegistry(TEST_UTIL.getConfiguration());
- RegionReplicaTestHelper.waitUntilAllMetaReplicasHavingRegionLocation(
+ RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(TEST_UTIL,
Review comment:
We pass in a TEST_UTIL AND a Configuration? Do we always get the configuration from TEST_UTIL? If so, just pass TEST_UTIL? Otherwise, ignore this comment.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369897054
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java
##########
@@ -133,11 +133,19 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception {
util.startMiniZKCluster();
util.createRootDir();
final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
+ new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class,
MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
final int MASTER_INDEX = 0;
final MasterThread master = cluster.getMasters().get(MASTER_INDEX);
master.start();
+ // Switching to master registry exposed a race in the master bootstrap that can result in a
+ // lost shutdown command (essentially HBASE-8422). The race is essentially because the
+ // server manager in HMaster is not initialized by the time shutdown() RPC (below) is made to
+ // the master. The reason it was not happening earlier is because the connection creation with
+ // ZK registry is so slow that by then the server manager is init'ed thus masking the problem.
+ // For now, I'm putting a sleep here to workaround the issue, I think the fix for it is a little
+ // delicate and needs to be done separately.
+ Thread.sleep(5000);
Review comment:
That is cleaner, done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-577427617
Reposting from the jira comment:
@apurtell TestFromClientSide was known to be flaky (even before this patch). So I'm fairly certain it has nothing to do with the current jira.
One thing @ndimiduk and I noticed here is that switching to JUnit 4.13 (HBASE-23664) has exacerbated the problem. Based on some initial debugging it looks like it has something to do with leaking FileSystem/DFSClient objects in the HBase code and restarting the minicluster in the same JUnit test runner JVM (what TestFromClientSide does) causes a leak of hdfs lease renewer threads. Some how Junit 4.12 was masking the problem.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370963189
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
Review comment:
Added the logging. Don't think it should be warn though.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370960778
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterShutdown.java
##########
@@ -132,12 +133,19 @@ public void testMasterShutdownBeforeStartingAnyRegionServer() throws Exception {
util.startMiniDFSCluster(3);
util.startMiniZKCluster();
util.createRootDir();
- final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
- MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
+ final LocalHBaseCluster cluster = new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS,
+ HMaster.class, MiniHBaseCluster.MiniHBaseClusterRegionServer.class);
final int MASTER_INDEX = 0;
final MasterThread master = cluster.getMasters().get(MASTER_INDEX);
master.start();
+ // Switching to master registry exposed a race in the master bootstrap that can result in a
+ // lost shutdown command (HBASE-8422). The race is essentially because the server manager in
+ // HMaster is not initialized by the time shutdown() RPC (below) is made to
+ // the master. The reason it was not happening earlier is because the connection creation with
+ // ZK registry is so slow that by then the server manager is init'ed thus masking the problem.
+ // For now, I'm putting a wait() here to workaround the issue, I think the fix for it is a
+ // little delicate and needs to be done separately.
Review comment:
I was thinking about fixing the actual problem rather than the test. If a shutdown happens before starting the server manager, the latter takes note of that during init and shuts down automatically (or something along the lines). My fix is in the master bootstrap.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369986062
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,17 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. RegionServers can continue be up independent of
+ // masters' availability.
+ // - Configuration management for region servers (cluster internal) is much simpler when adding
+ // new masters etc.
Review comment:
So if one is adding new masters to the cluster (or taking down an existing master), they will have to update the region servers config by doing a rolling restart (unless there is some dynamic way of updating it). This operational overhead is avoided since the regionservers figure it out from ZK. They just have to update it on the clients.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370880620
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAlwaysStandByHMaster.java
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import static junit.framework.TestCase.assertTrue;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.StartMiniClusterOption;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category({MediumTests.class, MasterTests.class})
+public class TestAlwaysStandByHMaster {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE =
+ HBaseClassTestRule.forClass(TestAlwaysStandByHMaster.class);
+ private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();
+
+ @BeforeClass
+ public static void setup() throws Exception {
Review comment:
Why no `MiniClusterRule`? 😢
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370883108
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -36,7 +36,7 @@ private ConnectionRegistryFactory() {
*/
static ConnectionRegistry getRegistry(Configuration conf) {
Class<? extends ConnectionRegistry> clazz = conf.getClass(
- CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, ZKConnectionRegistry.class,
+ CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, MasterRegistry.class,
Review comment:
It all pivots around this one little change :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369944939
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/AlwaysStandByMasterManager.java
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master;
+
+import java.io.IOException;
+import org.apache.hadoop.hbase.Server;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.monitoring.MonitoredTask;
+import org.apache.hadoop.hbase.util.Threads;
+import org.apache.hadoop.hbase.zookeeper.MasterAddressTracker;
+import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An implementation of ActiveMasterManager that never transitions it's master to active state. It
+ * always remains as a stand by master. With the master registry implementation (HBASE-18095) it is
+ * expected to have at least one active / standby master always running at any point in time since
+ * they serve as the gateway for client connections.
+ *
+ * With this implementation, tests can simulate the scenario of not having an active master yet the
+ * client connections to the cluster succeed.
+ */
+@InterfaceAudience.Private
+class AlwaysStandByMasterManager extends ActiveMasterManager {
+ private static final Logger LOG = LoggerFactory.getLogger(AlwaysStandByMasterManager.class);
+
+ AlwaysStandByMasterManager(ZKWatcher watcher, ServerName sn, Server master) {
+ super(watcher, sn, master);
+ }
+
+ /**
+ * An implementation that never transitions to an active master.
+ */
+ boolean blockUntilBecomingActiveMaster(int checkInterval, MonitoredTask startupStatus) {
+ while (!(master.isAborted() || master.isStopped())) {
+ startupStatus.setStatus("Forever looping to stay as a standby master.");
+ try {
+ activeMasterServerName = null;
+ try {
+ if (MasterAddressTracker.getMasterAddress(watcher) != null) {
+ clusterHasActiveMaster.set(true);
+ }
+ Threads.sleepWithoutInterrupt(100);
+ } catch (IOException e) {
+ // pass, we will get notified when some other active master creates the znode.
+ }
+ } catch (KeeperException e) {
+ master.abort("Received an unexpected KeeperException, aborting", e);
+ return false;
+ }
+ synchronized (this.clusterHasActiveMaster) {
+ while (clusterHasActiveMaster.get() && !master.isStopped()) {
+ try {
+ clusterHasActiveMaster.wait(checkInterval);
+ } catch (InterruptedException e) {
+ // We expect to be interrupted when a master dies,
+ // will fall out if so
+ LOG.debug("Interrupted waiting for master to die", e);
+ }
+ }
+ if (clusterShutDown.get()) {
+ this.master.stop(
+ "Cluster went down before this master became active");
+ }
+ }
+ }
+ return false;
+ }
+}
Review comment:
Good.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#issuecomment-577523063
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 3m 51s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 1s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 47 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 46s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 45s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 33s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 3s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 19s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 56s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 50s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 28s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 26s | hbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5) |
| +1 :green_heart: | checkstyle | 0m 34s | The patch passed checkstyle in hbase-client |
| +1 :green_heart: | checkstyle | 1m 35s | hbase-server: The patch generated 0 new + 451 unchanged - 2 fixed = 451 total (was 453) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 2s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 14s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 15s | the patch passed |
| +1 :green_heart: | findbugs | 7m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 3m 7s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 1m 54s | hbase-client in the patch passed. |
| -1 :x: | unit | 176m 2s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 16s | The patch does not generate ASF License warnings. |
| | | 258m 29s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.TestZooKeeper |
| | hadoop.hbase.replication.TestReplicationSmallTests |
| | hadoop.hbase.client.TestFromClientSide |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/6/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1039 |
| JIRA Issue | HBASE-23647 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 14c22c9438e5 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1039/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / d9bb034c94 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/6/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/6/testReport/ |
| Max. process+thread count | 5220 (vs. ulimit of 10000) |
| modules | C: hbase-common hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1039/6/console |
| versions | git=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11 |
| 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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370897102
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -36,7 +36,7 @@ private ConnectionRegistryFactory() {
*/
static ConnectionRegistry getRegistry(Configuration conf) {
Class<? extends ConnectionRegistry> clazz = conf.getClass(
- CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, ZKConnectionRegistry.class,
+ CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, MasterRegistry.class,
Review comment:
:D
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369893660
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
##########
@@ -268,14 +268,19 @@ public static Configuration createClusterConf(Configuration baseConf, String clu
* used to communicate with distant clusters
* @param conf configuration object to configure
* @param key string that contains the 3 required configuratins
- * @throws IOException
*/
private static void applyClusterKeyToConf(Configuration conf, String key)
- throws IOException{
+ throws IOException {
ZKConfig.ZKClusterKey zkClusterKey = ZKConfig.transformClusterKey(key);
conf.set(HConstants.ZOOKEEPER_QUORUM, zkClusterKey.getQuorumString());
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, zkClusterKey.getClientPort());
conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, zkClusterKey.getZnodeParent());
+ // Without the right registry, the above configs are useless. Also, we don't use setClass()
+ // here because the ConnectionRegistry* classes are not resolvable from this module.
+ // This will be broken if ZkConnectionRegistry class gets renamed or moved. Is there a better
+ // way?
Review comment:
> fall through to alternate or recovery code
There is no alternate or recovery from that point, no? The same error is propagated while creating the registry instance, so I guess we don't need to do it again here I 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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369935638
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -170,27 +171,36 @@ public LocalHBaseCluster(final Configuration conf, final int noMasters,
this.masterClass = (Class<? extends HMaster>)
conf.getClass(HConstants.MASTER_IMPL, masterClass);
// Start the HMasters.
- for (int i = 0; i < noMasters; i++) {
+ int i;
+ for (i = 0; i < noMasters; i++) {
addMaster(new Configuration(conf), i);
}
-
- // Populate the master address host ports in the config. This is needed if a master based
- // registry is configured for client metadata services (HBASE-18095)
- List<String> masterHostPorts = new ArrayList<>();
- getMasters().forEach(masterThread ->
- masterHostPorts.add(masterThread.getMaster().getServerName().getAddress().toString()));
- conf.set(HConstants.MASTER_ADDRS_KEY, String.join(",", masterHostPorts));
-
+ for (int j = 0; j < noAlwaysStandByMasters; j++) {
+ Configuration c = new Configuration(conf);
+ c.set(HConstants.MASTER_IMPL, "org.apache.hadoop.hbase.master.AlwaysStandByHMaster");
Review comment:
Late to the game , but what is an AlwaysStandByHMaster rather than StandByMaster? Why the Always? Maybe it will make sense later in review.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369985314
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,17 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. RegionServers can continue be up independent of
+ // masters' availability.
Review comment:
Region servers abort if the connection doesn't work for any reason. With master registry, if the master is down the registry methods throw connection refused and that can bringdown the entire set of region servers at the same time.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369940617
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,17 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. RegionServers can continue be up independent of
+ // masters' availability.
+ // - Configuration management for region servers (cluster internal) is much simpler when adding
+ // new masters etc.
Review comment:
I don't understand this one.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369998990
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenInitializing.java
##########
@@ -96,7 +96,7 @@ public void testRSTerminationAfterRegisteringToMasterBeforeCreatingEphemeralNode
TEST_UTIL.startMiniZKCluster();
TEST_UTIL.createRootDir();
final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
+ new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class,
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369940494
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,17 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. RegionServers can continue be up independent of
+ // masters' availability.
Review comment:
Is this not possible when Master Registry?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] bharathv commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370960515
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/RegionReplicaTestHelper.java
##########
@@ -43,24 +42,30 @@ private RegionReplicaTestHelper() {
}
// waits for all replicas to have region location
- static void waitUntilAllMetaReplicasHavingRegionLocation(Configuration conf,
- ConnectionRegistry registry, int regionReplication) throws IOException {
+ static void waitUntilAllMetaReplicasAreReady(HBaseTestingUtility util,
+ ConnectionRegistry registry, int regionReplication) {
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] ndimiduk commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r370876329
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
##########
@@ -3058,6 +3061,26 @@ private void initConnection() throws IOException {
this.asyncConnection = ClusterConnectionFactory.createAsyncClusterConnection(conf, null, user);
}
+ /**
+ * Resets the connections so that the next time getConnection() is called, a new connection is
+ * created. This is needed in cases where the entire cluster / all the masters are shutdown and
+ * the connection is not valid anymore.
+ * TODO: There should be a more coherent way of doing this. Unfortunately the way tests are
+ * written, not all start() stop() calls go through this class. Most tests directly operate on
+ * the underlying mini/local hbase cluster. That makes it difficult for this wrapper class to
+ * maintain the connection state automatically. Cleaning this is a much bigger refactor.
Review comment:
> Cleaning this is a much bigger refactor.
Ugh.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369945441
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRSKilledWhenInitializing.java
##########
@@ -96,7 +96,7 @@ public void testRSTerminationAfterRegisteringToMasterBeforeCreatingEphemeralNode
TEST_UTIL.startMiniZKCluster();
TEST_UTIL.createRootDir();
final LocalHBaseCluster cluster =
- new LocalHBaseCluster(conf, NUM_MASTERS, NUM_RS, HMaster.class,
+ new LocalHBaseCluster(conf, NUM_MASTERS, 0, NUM_RS, HMaster.class,
Review comment:
We seem to do '0' as AlwaysMasters. Should we do override that defaults zero? Just to keep the AlwaysMasters out of view when not needed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] apurtell merged pull request #1039: HBASE-23647: Make
MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
apurtell merged pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369943408
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/StartMiniClusterOption.java
##########
@@ -46,6 +46,14 @@
* can find the active/primary master with {@link MiniHBaseCluster#getMaster()}.
*/
private final int numMasters;
+
+ /**
+ * Number of masters that always remain standby. These set of masters never transition to active
+ * even if an active master does not exist. These are needed for testing scenarios where there are
+ * no active masters in the cluster but the cluster connection (backed by master registry) should
+ * still work.
+ */
+ private final int numAlwaysStandByMasters;
Review comment:
Would be good to keep them 'hidden' as much as we can.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369943797
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncAdminMasterSwitch.java
##########
@@ -48,8 +48,6 @@ public void testSwitch() throws IOException, InterruptedException {
assertEquals(TEST_UTIL.getHBaseCluster().getRegionServerThreads().size(),
admin.getClusterMetrics(EnumSet.of(ClusterMetrics.Option.SERVERS_NAME)).join()
.getServersName().size());
- // stop the old master, and start a new one
- TEST_UTIL.getMiniHBaseCluster().startMaster();
Review comment:
No need of a new master to complete test?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [hbase] apurtell commented on a change in pull request #1039:
HBASE-23647: Make MasterRegistry the default impl.
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #1039: HBASE-23647: Make MasterRegistry the default impl.
URL: https://github.com/apache/hbase/pull/1039#discussion_r369862230
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -789,8 +789,19 @@ public boolean registerService(com.google.protobuf.Service instance) {
return true;
}
- private Configuration unsetClientZookeeperQuorum() {
+ private Configuration cleanupConfiguration() {
Configuration conf = this.conf;
+ // We use ZKConnectionRegistry for all the internal communication, primarily for these reasons:
+ // - Decouples RS and master life cycles. For example, if all the masters are down, region
+ // servers can abort at the same time, because the internal connection is master dependent and
+ // fails.This is an operational nightmare. Using the ZK based registry means that the region
+ // servers are now on the look out for new masters, if they are spun up.
+ // - Configuration management for region servers (cluster internal) is much simpler when adding
+ // new masters etc.
+ // - We need to retain ZKConnectionRegistry for replication use anyway, so we just extend it for
+ // other internal connections too.
+ conf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
+ "org.apache.hadoop.hbase.client.ZKConnectionRegistry");
Review comment:
Ok, but these class names can be constant strings at least.
----------------------------------------------------------------
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
With regards,
Apache Git Services