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 2019/12/20 23:38:18 UTC
[GitHub] [hbase] bharathv opened a new pull request #957: HBASE-23604:
Clarify AsyncRegistry usage in the code.
bharathv opened a new pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957
This patch is a simple refactor that renames a bunch of classes
to add more context to the readers. The code is littered with
the usage of word "registry" but it is not clear what the registry
is all about. This patch renames a bunch of classes to make it
more self explanatory.
----------------------------------------------------------------
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 issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569444673
I think the rename to ConnectionRegistry looks 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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570470609
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 23s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 23 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 54s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 25s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 1s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 1s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 1s | 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 | 5m 54s | 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 37s | the patch passed |
| +1 :green_heart: | compile | 1m 25s | the patch passed |
| +1 :green_heart: | javac | 1m 25s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 34s | hbase-client: The patch generated 0 new + 14 unchanged - 2 fixed = 14 total (was 16) |
| +1 :green_heart: | checkstyle | 1m 29s | The patch passed checkstyle in hbase-server |
| +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 | 18m 40s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 2s | the patch passed |
| +1 :green_heart: | findbugs | 6m 45s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 4s | hbase-client in the patch passed. |
| -1 :x: | unit | 193m 58s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 6s | The patch does not generate ASF License warnings. |
| | | 269m 8s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient |
| | hadoop.hbase.security.access.TestSnapshotScannerHDFSAclController |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/957 |
| JIRA Issue | HBASE-23604 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 5e0cae99f245 4.15.0-70-generic #79-Ubuntu SMP Tue Nov 12 10:36:11 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-957/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / dffa9be899 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/5/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/5/testReport/ |
| Max. process+thread count | 4960 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/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] virajjasani edited a comment on issue #957: HBASE-23604:
Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570634479
> @virajjasani you have anything to add here?
changes look good and went through the conversations, +1 from my side
----------------------------------------------------------------
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 issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570308068
Thank you for making an effort to improve this class name. I also find it confusing for a first-read experience.
----------------------------------------------------------------
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 issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569444590
I rebased for you. Why the conflict above do you think? And do you want to fix the checkstyle?
----------------------------------------------------------------
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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570426413
@saintstack / @ndimiduk I force pushed after rebasing on the latest HEAD. Can one of you please merge this if it is good to go? I would like to rebase my registry patch on top of this.
----------------------------------------------------------------
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] virajjasani commented on issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
virajjasani commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570634479
> @virajjasani you have anything to add here?
changes look good, +1 from my side
----------------------------------------------------------------
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 #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r362678988
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -18,26 +18,28 @@
package org.apache.hadoop.hbase.client;
import org.apache.hadoop.conf.Configuration;
-import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.util.ReflectionUtils;
+import org.apache.yetus.audience.InterfaceAudience;
/**
- * Get instance of configured Registry.
+ * Factory class to get the instance of configured connection registry.
*/
@InterfaceAudience.Private
-final class AsyncRegistryFactory {
+final class ConnectionRegistryFactory {
- static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl";
+ static final String CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY =
+ "hbase.client.connection.registry.impl";
Review comment:
Bleh. "ClusterInternalServiceDiscoveryService"? I give up and resign myself to "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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570656926
> TestZooKeeperTableArchiveClient fails consistently for me. Looks like it's built around a mock registry instance. Mind taking a look first?
@ndimiduk Oops missed it. Was a legit bug. Fixed it. NPE shows up because the CONNECTION was not set up correctly (and hence == null) and we are trying to do a null.close(). Fixed the underlying bug. Verified locally that the test passes.
----------------------------------------------------------------
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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-568200519
> 'meta' is a loaded term in hbase and in particular, usually refers to the meta table.
That is a fair point. There are definitely other connotations for the word "meta" in this project and readers might get confused even more.
> There are some good changes in this patch -- the getActiveMasters and making meta locations plural. Get them in? And meantime we all 'learn' what Registry is about...
Ya. I can't think of a good name yet, without the use of word meta. (ConnectionRegistry?). I'm undoing the class renames and (force) pushing the change again. May be we can revisit later.
----------------------------------------------------------------
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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-568216368
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | 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 6 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 19s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 21s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 1m 51s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 42s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 59s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 28s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 5m 31s | 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 | 5m 0s | the patch passed |
| +1 :green_heart: | compile | 1m 22s | the patch passed |
| +1 :green_heart: | javac | 1m 22s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 33s | hbase-client: The patch generated 0 new + 14 unchanged - 1 fixed = 14 total (was 15) |
| +1 :green_heart: | checkstyle | 1m 16s | The patch passed checkstyle in hbase-server |
| +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 56s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 0s | the patch passed |
| +1 :green_heart: | findbugs | 5m 43s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 52s | hbase-client in the patch passed. |
| -1 :x: | unit | 180m 32s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 1m 6s | The patch does not generate ASF License warnings. |
| | | 246m 51s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.master.assignment.TestOpenRegionProcedureHang |
| | hadoop.hbase.regionserver.TestSplitTransactionOnCluster |
| | hadoop.hbase.quotas.TestQuotaAdmin |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/957 |
| JIRA Issue | HBASE-23604 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux b4ef8b6d0fd4 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-957/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / e41b46cc28 |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/2/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/2/testReport/ |
| Max. process+thread count | 4627 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/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] saintstack commented on a change in pull request #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r362652354
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -18,26 +18,28 @@
package org.apache.hadoop.hbase.client;
import org.apache.hadoop.conf.Configuration;
-import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.util.ReflectionUtils;
+import org.apache.yetus.audience.InterfaceAudience;
/**
- * Get instance of configured Registry.
+ * Factory class to get the instance of configured connection registry.
*/
@InterfaceAudience.Private
-final class AsyncRegistryFactory {
+final class ConnectionRegistryFactory {
- static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl";
+ static final String CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY =
+ "hbase.client.connection.registry.impl";
Review comment:
I like this last take of @bharathv -- don't like it either but does convey one-stop-shop.
Service is overloaded usually a protobuf Service implemenation such as Admin or Client but also guava Service implementations done internally. If we did Service -- while it might be interesting to expose hbase as a java Service -- I think stuff would get a little confusing when discovery tier talks of Service and then the lower rpc tier talks of another Service type altogether. Locator might work but fails like Service in not being comprehensive enough to cover all exposed in the Interface. Its internal at the moment used in a few places.
Registry while ugly seems the better 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] ndimiduk merged pull request #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk merged pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957
----------------------------------------------------------------
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 #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r362585035
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistry.java
##########
@@ -24,16 +24,17 @@
import org.apache.yetus.audience.InterfaceAudience;
/**
- * Implementations hold cluster information such as this cluster's id, location of hbase:meta, etc..
+ * Registry for meta information needed for connection setup to a HBase cluster. Implementations
+ * hold cluster information such as this cluster's id, location of hbase:meta, etc..
* Internal use only.
*/
@InterfaceAudience.Private
-interface AsyncRegistry extends Closeable {
+interface ConnectionRegistry extends Closeable {
Review comment:
Why is this still called a "registry"?
A name we don't use much internally, but I think is more appropriate for this concept is a "service". What we have here follows exactly the definitions provided by the java docs on [`java.util.ServiceLoader`](https://docs.oracle.com/javase/7/docs/api/java/util/ServiceLoader.html),
> A _service_ is a well-known set of interfaces and (usually abstract) classes. A service _provider_ is a specific implementation of a service.
This implementation provides the "cluster/connection/master location service". Thus, I prefer the name "ClusterLocationService".
If you don't like the name "service", we use the term "Locator" in plenty of other places in this codebase, so why not here? Implementations of this class "locate" the active master and "locate" the meta regions. The `clusterId` is the only concept that isn't "locatable", but I think that's not so bad. How about "ClusterLocator" or "ClusterConnectionLocator" ?
----------------------------------------------------------------
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 #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r362683999
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -18,26 +18,28 @@
package org.apache.hadoop.hbase.client;
import org.apache.hadoop.conf.Configuration;
-import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.util.ReflectionUtils;
+import org.apache.yetus.audience.InterfaceAudience;
/**
- * Get instance of configured Registry.
+ * Factory class to get the instance of configured connection registry.
*/
@InterfaceAudience.Private
-final class AsyncRegistryFactory {
+final class ConnectionRegistryFactory {
- static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl";
+ static final String CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY =
+ "hbase.client.connection.registry.impl";
Review comment:
lol :-)
----------------------------------------------------------------
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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569974570
@saintstack Can you please merge this when you get a chance? Will rebase #954. Thanks for the reviews.
----------------------------------------------------------------
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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569390356
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 3m 55s | 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 23 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 57s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 23s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 3s | 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 4s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 49s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 5m 57s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| -0 :warning: | patch | 5m 4s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 33s | the patch passed |
| +1 :green_heart: | compile | 1m 24s | the patch passed |
| +1 :green_heart: | javac | 1m 24s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 32s | hbase-client: The patch generated 0 new + 14 unchanged - 2 fixed = 14 total (was 16) |
| -1 :x: | checkstyle | 1m 27s | hbase-server: The patch generated 1 new + 1 unchanged - 0 fixed = 2 total (was 1) |
| +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 48s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 0m 59s | the patch passed |
| +1 :green_heart: | findbugs | 6m 15s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 51s | hbase-client in the patch passed. |
| -1 :x: | unit | 321m 1s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 52s | The patch does not generate ASF License warnings. |
| | | 396m 19s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.master.procedure.TestSCPWithReplicasWithoutZKCoordinated |
| | hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient |
| | hadoop.hbase.client.TestAdmin2 |
| | hadoop.hbase.master.TestSplitWALManager |
| | hadoop.hbase.client.TestSnapshotTemporaryDirectoryWithRegionReplicas |
| | 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-957/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/957 |
| JIRA Issue | HBASE-23604 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 073bd2f97bda 4.15.0-70-generic #79-Ubuntu SMP Tue Nov 12 10:36:11 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-957/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / e41b46cc28 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/3/artifact/out/diff-checkstyle-hbase-server.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/3/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/3/testReport/ |
| Max. process+thread count | 4894 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/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] Apache-HBase commented on issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570714356
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | 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 1s | The patch appears to include 23 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 16s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 16s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 1m 49s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 40s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 1s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 24s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 5m 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 | 5m 0s | the patch passed |
| +1 :green_heart: | compile | 1m 24s | the patch passed |
| +1 :green_heart: | javac | 1m 24s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 31s | hbase-client: The patch generated 0 new + 14 unchanged - 2 fixed = 14 total (was 16) |
| +1 :green_heart: | checkstyle | 1m 17s | The patch passed checkstyle in hbase-server |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 32s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 16m 4s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 1m 0s | the patch passed |
| +1 :green_heart: | findbugs | 5m 56s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 59s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 157m 21s | hbase-server in the patch passed. |
| +1 :green_heart: | asflicense | 1m 6s | The patch does not generate ASF License warnings. |
| | | 223m 38s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/6/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/957 |
| JIRA Issue | HBASE-23604 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 49619b10215a 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-957/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / dffa9be899 |
| Default Java | 1.8.0_181 |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/6/testReport/ |
| Max. process+thread count | 4779 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/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 #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r362591422
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -18,26 +18,28 @@
package org.apache.hadoop.hbase.client;
import org.apache.hadoop.conf.Configuration;
-import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.util.ReflectionUtils;
+import org.apache.yetus.audience.InterfaceAudience;
/**
- * Get instance of configured Registry.
+ * Factory class to get the instance of configured connection registry.
*/
@InterfaceAudience.Private
-final class AsyncRegistryFactory {
+final class ConnectionRegistryFactory {
- static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl";
+ static final String CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY =
+ "hbase.client.connection.registry.impl";
Review comment:
I'm not a big fan of the word "registry" either but I think it signifies that it is a one stop shop for all the information needed for connection related metadata (which is probably why it was chosen in the first place). Also, like you said, it has been used in the configs for a while, so...
----------------------------------------------------------------
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 #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r360654153
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -257,7 +257,7 @@ NonceGenerator getNonceGenerator() {
CompletableFuture<MasterService.Interface> getMasterStub() {
return ConnectionUtils.getOrFetch(masterStub, masterStubMakeFuture, false, () -> {
CompletableFuture<MasterService.Interface> future = new CompletableFuture<>();
- addListener(registry.getMasterAddress(), (addr, error) -> {
+ addListener(registry.getActiveMaster(), (addr, error) -> {
Review comment:
This is 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] ndimiduk commented on issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570639097
@bharathv `TestZooKeeperTableArchiveClient` fails consistently for me. Looks like it's built around a mock registry instance. Mind taking a look first?
----------------------------------------------------------------
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 issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570639921
From [patch-unit-hbase-server.txt](https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/5/artifact/out/patch-unit-hbase-server.txt), I see
```
[ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.118 s <<< FAILURE! - in org.apache.hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient
[ERROR] org.apache.hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient Time elapsed: 0.012 s <<< ERROR!
java.io.IOException: org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /hbase/hbaseid
at org.apache.hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient.setupCluster(TestZooKeeperTableArchiveClient.java:115)
Caused by: org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /hbase/hbaseid
[ERROR] org.apache.hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient Time elapsed: 0.013 s <<< ERROR!
java.lang.NullPointerException
at org.apache.hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient.cleanupTest(TestZooKeeperTableArchiveClient.java:150)
```
----------------------------------------------------------------
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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569491955
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 37s | 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 23 new or modified test files. |
||| _ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _ |
| +0 :ok: | mvndep | 1m 55s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 6m 33s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 23s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 1m 54s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 4m 44s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 5s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 20s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 5m 32s | 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 | 5m 2s | the patch passed |
| +1 :green_heart: | compile | 1m 23s | the patch passed |
| +1 :green_heart: | javac | 1m 23s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 32s | hbase-client: The patch generated 0 new + 14 unchanged - 2 fixed = 14 total (was 16) |
| +1 :green_heart: | checkstyle | 1m 16s | The patch passed checkstyle in hbase-server |
| +1 :green_heart: | whitespace | 0m 1s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 4m 36s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 16m 5s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 0m 58s | the patch passed |
| +1 :green_heart: | findbugs | 5m 50s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 2s | hbase-client in the patch passed. |
| -1 :x: | unit | 163m 40s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 57s | The patch does not generate ASF License warnings. |
| | | 233m 21s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.backup.example.TestZooKeeperTableArchiveClient |
| | hadoop.hbase.master.assignment.TestRegionReplicaSplit |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/957 |
| JIRA Issue | HBASE-23604 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux f8a2f9161d66 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-957/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / 1c41b3652b |
| Default Java | 1.8.0_181 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/4/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/4/testReport/ |
| Max. process+thread count | 4725 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/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] bharathv commented on issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569364159
@saintstack I also pushed a commit that does the rename of AsyncRegistry -> ConnectionRegistry. Let me know what you think. I thought it'd be good to push both the commits together.
----------------------------------------------------------------
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 issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569338692
i.e. you need the Registry to make a useful Connection to 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 issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-570634067
@virajjasani you have anything to add 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] saintstack commented on a change in pull request #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r360654175
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionMetaRegistry.java
##########
@@ -24,16 +24,17 @@
import org.apache.yetus.audience.InterfaceAudience;
/**
- * Implementations hold cluster information such as this cluster's id, location of hbase:meta, etc..
+ * Registry for meta information needed for connection setup to a HBase cluster. Implementations
+ * hold cluster information such as this cluster's id, location of hbase:meta, etc..
* Internal use only.
*/
@InterfaceAudience.Private
-interface AsyncRegistry extends Closeable {
+interface AsyncConnectionMetaRegistry extends Closeable {
/**
- * Get the location of meta region.
+ * Get the current locations of meta region(s).
*/
- CompletableFuture<RegionLocations> getMetaRegionLocation();
+ CompletableFuture<RegionLocations> getMetaRegionLocations();
Review comment:
This is 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 #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-568156141
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 24s | 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 23 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 43s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | compile | 1m 25s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | checkstyle | 2m 3s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +1 :green_heart: | shadedjars | 5m 4s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 5s | HBASE-18095/client-locate-meta-no-zookeeper passed |
| +0 :ok: | spotbugs | 4m 52s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 6m 1s | HBASE-18095/client-locate-meta-no-zookeeper passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 19s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 31s | the patch passed |
| +1 :green_heart: | compile | 1m 24s | the patch passed |
| +1 :green_heart: | javac | 1m 24s | the patch passed |
| -1 :x: | checkstyle | 0m 34s | hbase-client: The patch generated 35 new + 15 unchanged - 1 fixed = 50 total (was 16) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 5m 6s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 17m 40s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2. |
| +1 :green_heart: | javadoc | 0m 59s | the patch passed |
| +1 :green_heart: | findbugs | 6m 13s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 51s | hbase-client in the patch passed. |
| -1 :x: | unit | 313m 25s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 55s | The patch does not generate ASF License warnings. |
| | | 386m 0s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.master.procedure.TestSCPWithReplicas |
| | hadoop.hbase.master.TestSplitWALManager |
| | hadoop.hbase.client.TestSnapshotTemporaryDirectoryWithRegionReplicas |
| | hadoop.hbase.master.TestAssignmentManagerMetrics |
| | hadoop.hbase.client.TestFromClientSide3 |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/957 |
| JIRA Issue | HBASE-23604 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux c2ad08fb052e 4.15.0-70-generic #79-Ubuntu SMP Tue Nov 12 10:36:11 UTC 2019 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-957/out/precommit/personality/provided.sh |
| git revision | HBASE-18095/client-locate-meta-no-zookeeper / e41b46cc28 |
| Default Java | 1.8.0_181 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/1/artifact/out/diff-checkstyle-hbase-client.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/1/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/1/testReport/ |
| Max. process+thread count | 4849 (vs. ulimit of 10000) |
| modules | C: hbase-client hbase-server U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-957/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] bharathv commented on issue #957: HBASE-23604: Clarify
AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
bharathv commented on issue #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#issuecomment-569339987
Thanks for the review.
> i.e. you need the Registry to make a useful Connection to the cluster.
Yea, that seems meaningful and simple enough to me as well. Will do a follow up.
>Where should I merge?
Can you please merge this into the feature branch? 'HBASE-18095/client-locate-meta-no-zookeeper'. While you are here, can you please rebase the feature branch with latest master? I checked that the rebase is clean but I don't have the push rights.
----------------------------------------------------------------
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 #957:
HBASE-23604: Clarify AsyncRegistry usage in the code.
Posted by GitBox <gi...@apache.org>.
ndimiduk commented on a change in pull request #957: HBASE-23604: Clarify AsyncRegistry usage in the code.
URL: https://github.com/apache/hbase/pull/957#discussion_r362585302
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionRegistryFactory.java
##########
@@ -18,26 +18,28 @@
package org.apache.hadoop.hbase.client;
import org.apache.hadoop.conf.Configuration;
-import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.util.ReflectionUtils;
+import org.apache.yetus.audience.InterfaceAudience;
/**
- * Get instance of configured Registry.
+ * Factory class to get the instance of configured connection registry.
*/
@InterfaceAudience.Private
-final class AsyncRegistryFactory {
+final class ConnectionRegistryFactory {
- static final String REGISTRY_IMPL_CONF_KEY = "hbase.client.registry.impl";
+ static final String CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY =
+ "hbase.client.connection.registry.impl";
Review comment:
Oh no! We call it a "registry" in our configs ☹️ This means we're stuck with the name "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