Skip to content

HBASE-23305: Master based registry implementation - #954

Merged
ndimiduk merged 1 commit into
apache:HBASE-18095/client-locate-meta-no-zookeeperfrom
bharathv:HBASE-23305
Jan 14, 2020
Merged

HBASE-23305: Master based registry implementation#954
ndimiduk merged 1 commit into
apache:HBASE-18095/client-locate-meta-no-zookeeperfrom
bharathv:HBASE-23305

Conversation

@bharathv

Copy link
Copy Markdown
Contributor

Implements a master based registry for clients.

  • Supports hedged RPCs (fan out configured via configs).
  • Parameterized existing client tests to run with multiple
    registry combinations.
  • Added unit-test coverage for the new registry implenenation.

@bharathvbharathv left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apurtell@ndimiduk@saintstack Next in the series of patches that implements a master registry.

This patch is a little more involved than the earlier ones. Happy to do anything that makes it easier for you to review. Let me know.

I'm still thinking of ways to improve the test coverage, especially injecting faults and timeouts. Even without that, I think the patch is ready for review. Any feedback welcome.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 11sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 1sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 8 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 35sMaven dependency ordering for branch
+1 💚mvninstall5m 52sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile1m 44sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 34sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars5m 3sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 19sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs4m 48sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs6m 42sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 14sMaven dependency ordering for patch
+1 💚mvninstall5m 29sthe patch passed
+1 💚compile1m 44sthe patch passed
+1 💚javac1m 44sthe patch passed
-1 ❌checkstyle1m 33shbase-server: The patch generated 2 new + 224 unchanged - 45 fixed = 226 total (was 269)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
-1 ❌shadedjars2m 42spatch has 10 errors when building our shaded downstream artifacts.
+1 💚hadoopcheck17m 26sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
-1 ❌javadoc0m 21shbase-client generated 2 new + 2 unchanged - 0 fixed = 4 total (was 2)
-1 ❌findbugs1m 18shbase-client generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
_ Other Tests _
+1 💚unit3m 1shbase-common in the patch passed.
+1 💚unit1m 49shbase-client in the patch passed.
-1 ❌unit205m 58shbase-server in the patch failed.
-1 ❌asflicense1m 26sThe patch generated 2 ASF License warnings.
283m 25s
ReasonTests
FindBugsmodule:hbase-client
Should org.apache.hadoop.hbase.client.MasterRegistry$BatchRpcCtx be a static inner class? At MasterRegistry.java:inner class? At MasterRegistry.java:[lines 100-145]
Failed junit testshadoop.hbase.client.TestAsyncRegionAdminApi2
hadoop.hbase.client.TestAsyncTableRegionReplicasScan
hadoop.hbase.client.TestScannersFromClientSide
hadoop.hbase.client.TestMetaWithReplicas
hadoop.hbase.client.TestAsyncResultScannerCursor
hadoop.hbase.replication.TestReplicationDisableInactivePeer
hadoop.hbase.client.TestZKAsyncRegistry
hadoop.hbase.client.TestCIPutOperationTimeout
hadoop.hbase.client.TestCloneSnapshotFromClientAfterSplittingRegion
hadoop.hbase.client.TestMultipleTimestamps
hadoop.hbase.client.TestAdmin3
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux 55f8f10fe76f 4.15.0-66-generic #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / e41b46c
Default Java1.8.0_181
checkstylehttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/diff-checkstyle-hbase-server.txt
shadedjarshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/patch-shadedjars.txt
javadochttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/diff-javadoc-javadoc-hbase-client.txt
findbugshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/new-findbugs-hbase-client.html
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/testReport/
asflicensehttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/artifact/out/patch-asflicense-problems.txt
Max. process+thread count9918 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/1/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@saintstacksaintstack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch looks good. Need to add delay for hedged reading but that can come in follow-on.

We are definetly saying goodbye to Master being able to sit at the back of the class being able to check in just once-in-a-while... and even being able to check out for long stretches at a time. It is now critical inline factor in overall uptime. In exchange we get simplified cluster deploy and tighter control of the interface we present clients.

// Configured list of masters to probe the meta information from.
private final List<ServerName> masterServers;
// Controls the fan out of the hedged requests. Requests are made in batches of this number until
// all the servers are exhausted. The first returned result is passed back to the client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have a successful answer, do we kill/interrupt the other ongoing queries? Wondering because 100k clients going against 3 or 5 Masters will be a bit of a load.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If N connections to the cluster, how many MasterRegistries? As many as there are Connections? Or is there just a single instance per JVM and it is shared across?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my earlier review I suggest we do fan out adaptively by default. If single requests are performing adequately, fanout is unnecessary load for no reward.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have a successful answer, do we kill/interrupt the other ongoing queries? Wondering because 100k clients going against 3 or 5 Masters will be a bit of a load.

We interrupt the threads. I clarified it in the new design and wrote a detailed javadoc. Let me know if it is not clear.

If N connections to the cluster, how many MasterRegistries? As many as there are Connections? Or is there just a single instance per JVM and it is shared across?

This is actually a very good point. It is not once per JVM (although, I think that makes more sense to me). Infact it is multiple instances per connection (look at the callers of AsyncRegistryFactory#getRegistry()). I think there is definitely a scope for cleanup here. Can i revisit this as a follow up?

In my earlier review I suggest we do fan out adaptively by default. If single requests are performing adequately, fanout is unnecessary load for no reward.

Totally agree. I think the hedging policy needs to be smart. I clarified this in the new patch set. Can I implement this in a follow up patch? This is already too big.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infact it is multiple instances per connection (look at the callers of AsyncRegistryFactory#getRegistry()). I think there is definitely a scope for cleanup here. Can i revisit this as a follow up?

This is the state of things before you arrived, right? I'd say file this as a separate ticket, as another cleanup project for rebase, or for after this code lands. I think I'd prefer to see the former, but it's not a strong preference ATM. I do consider it a blocker for back port to a branch-2, however.

// RPC client used to talk to the masters.
private final RpcClient rpcClient;
private final RpcControllerFactory rpcControllerFactory;
private final int rpcTimeoutNs;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this timeout relate to general client rpc timeout? If I set rpc timeout for client of 10 seconds, is this timeout subsumed by the general client timeout or does this run indepentent of whatever the overall client timeout setting is?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned this up. Does not exist any more.

// at doRPCs().
@VisibleForTesting
@FunctionalInterface
public interface RpcCall<RESP> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is like AsyncAdminRequestRetryingCaller#Callable only it throws a ServiceException. Should it take a Controller? Should it be in a class of its own?

On ClientMetaService, 'meta' is overloaded in hbase. Usually it is about the hbase:meta table but here it is about something else? Should it be called something other than ClientMetaService? Somethign to do w/ Registry?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it return CompleteableFuture since we in async land?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this and implemented a generic hedging framework for any RPC at the rpc layer.

*/
private void parseMasterAddrs(Configuration conf) {
String configuredMasters = conf.get(MASTER_ADDRS_KEY, MASTER_ADDRS_DEFAULT);
for (String masterAddr: configuredMasters.split(",")) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Fragile? If space after comma, this breaks?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. Added some test coverage too.

masterServers.add(ServerName.valueOf(masterAddr, ServerName.NON_STARTCODE));
}
// (Pseudo) Randomized so that not all clients hot spot the same set of masters.
Collections.shuffle(masterServers);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good

conf.getInt(MASTER_REGISTRY_NUM_HEDGED_REQS_KEY, MASTER_REGISTRY_NUM_HEDGED_REQS_DEFAULT);
Preconditions.checkArgument(requestFanOut >= 1);
if (requestFanOut > 1) {
masterRpcPool = Executors.newFixedThreadPool(requestFanOut,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: delay query of second and third masters?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@apurtellapurtell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got to the point where I suggested a big refactor of the patch's approach and stopped there for now.

public class MasterRegistryFetchException extends HBaseIOException {
public MasterRegistryFetchException(List<ServerName> masters, String failedRPC) {
super(String.format("Exception making rpc %s to masters %s", failedRPC,
masters.stream().map(Objects::toString).collect(Collectors.toList())));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary Java 8 idioms

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much of this work has these, all presenting issues for backport (not insurmountable, though), but this one is less readable than a simple string join using Collections.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to PrettyPrinter as a general util for any collection.

but this one is less readable than a simple string join using Collections.

Fair point. Switched to good old better performing loops.

@VisibleForTesting
<RESP> Optional<RESP> doRPCs(RpcCall<RESP> rpcCall,
Function<RESP, Boolean> isvalidResp, String debug) {
if (requestFanOut == 1) {

@apurtellapurtellDec 20, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting for the first cut we'd do just one request to a random host on the list at a time, and retry with another random choice. (So above randomization of list is good and important.) This is what the zookeeper client does now so is no different from current state of play.

Hedged reading is ahead of the game.

Good that it is off by default, though.

Also, it's nice that fan out factor is configurable, but I would want an adaptive policy by default. Only if single requests are failing at some threshold of unacceptable probability (i suppose controlled by a config param) would you want to start loading up more than one per request in trade for faster response, hopefully, on average.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's generalize this and apply it to and reuse the existing RPC controller, RPC client, Callable, Call, Caller hierarchy.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Re-did the whole thing. Now the hedging happens in the RPC layer and is not specific to MasterRegistry anymore.

Preconditions.checkState(requestFanOut > 1);
Preconditions.checkNotNull(masterRpcPool);
int i = 0;
while (i < masterServers.size()){

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment regarding adaptive behavior. I'd prefer if we have fan out, to not blindly do it if single requests are performing adequately.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

while (i < masterServers.size()){
// Each iteration of loop picks requestFanOut masters
int subListSize = Math.min(masterServers.size(), i + requestFanOut);
List<ServerName> masterSubList = masterServers.subList(i, subListSize);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the list be randomized again? Or we're hitting the sublists deterministically. Make a private list at top of function and shuffle?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it'd be non-deterministic at the registry level. (different connections have different registries anyway, so wondering if we need to shuffle again.

private final Function<RESP, Boolean> isValidResp;
private final String debugStr;

MasterRpc(BatchRpcCtx<RESP> rpcCtx, ServerName master, RpcCall<RESP> rpcCall,

@apurtellapurtellDec 20, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a new one, right? Use existing RPC/Call facilities and set a retry policy where there are no retries using the factory methods for that.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 8sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 8 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 35sMaven dependency ordering for branch
+1 💚mvninstall5m 48sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile1m 45sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 32sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars5m 5sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 16sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs4m 46sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs6m 39sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 13sMaven dependency ordering for patch
+1 💚mvninstall5m 31sthe patch passed
+1 💚compile1m 46sthe patch passed
+1 💚javac1m 46sthe patch passed
+1 💚checkstyle0m 25sThe patch passed checkstyle in hbase-common
+1 💚checkstyle0m 34sThe patch passed checkstyle in hbase-client
+1 💚checkstyle1m 32shbase-server: The patch generated 0 new + 223 unchanged - 46 fixed = 223 total (was 269)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars5m 4spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck17m 29sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚javadoc1m 15sthe patch passed
-1 ❌findbugs1m 18shbase-client generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
_ Other Tests _
+1 💚unit3m 2shbase-common in the patch passed.
+1 💚unit1m 49shbase-client in the patch passed.
-1 ❌unit267m 40shbase-server in the patch failed.
+1 💚asflicense1m 22sThe patch does not generate ASF License warnings.
346m 53s
ReasonTests
FindBugsmodule:hbase-client
Should org.apache.hadoop.hbase.client.MasterRegistry$BatchRpcCtx be a static inner class? At MasterRegistry.java:inner class? At MasterRegistry.java:[lines 117-162]
Failed junit testshadoop.hbase.client.TestSplitOrMergeStatus
hadoop.hbase.client.TestFromClientSideWithCoprocessor
hadoop.hbase.master.TestMasterShutdown
hadoop.hbase.client.TestScannersFromClientSide
hadoop.hbase.client.TestMobSnapshotCloneIndependence
hadoop.hbase.client.TestCIPutOperationTimeout
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/2/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux 2c688ac1523d 4.15.0-66-generic #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / e41b46c
Default Java1.8.0_181
findbugshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/2/artifact/out/new-findbugs-hbase-client.html
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/2/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/2/testReport/
Max. process+thread count9917 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/2/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@bharathvbharathv left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-implemented the whole hedging logic again. Will force-push the new set of commits shortly.

// Configured list of masters to probe the meta information from.
private final List<ServerName> masterServers;
// Controls the fan out of the hedged requests. Requests are made in batches of this number until
// all the servers are exhausted. The first returned result is passed back to the client.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we have a successful answer, do we kill/interrupt the other ongoing queries? Wondering because 100k clients going against 3 or 5 Masters will be a bit of a load.

We interrupt the threads. I clarified it in the new design and wrote a detailed javadoc. Let me know if it is not clear.

If N connections to the cluster, how many MasterRegistries? As many as there are Connections? Or is there just a single instance per JVM and it is shared across?

This is actually a very good point. It is not once per JVM (although, I think that makes more sense to me). Infact it is multiple instances per connection (look at the callers of AsyncRegistryFactory#getRegistry()). I think there is definitely a scope for cleanup here. Can i revisit this as a follow up?

In my earlier review I suggest we do fan out adaptively by default. If single requests are performing adequately, fanout is unnecessary load for no reward.

Totally agree. I think the hedging policy needs to be smart. I clarified this in the new patch set. Can I implement this in a follow up patch? This is already too big.

*/
private void parseMasterAddrs(Configuration conf) {
String configuredMasters = conf.get(MASTER_ADDRS_KEY, MASTER_ADDRS_DEFAULT);
for (String masterAddr: configuredMasters.split(",")) {

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. Added some test coverage too.

@VisibleForTesting
<RESP> Optional<RESP> doRPCs(RpcCall<RESP> rpcCall,
Function<RESP, Boolean> isvalidResp, String debug) {
if (requestFanOut == 1) {

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Re-did the whole thing. Now the hedging happens in the RPC layer and is not specific to MasterRegistry anymore.

Preconditions.checkState(requestFanOut > 1);
Preconditions.checkNotNull(masterRpcPool);
int i = 0;
while (i < masterServers.size()){

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

while (i < masterServers.size()){
// Each iteration of loop picks requestFanOut masters
int subListSize = Math.min(masterServers.size(), i + requestFanOut);
List<ServerName> masterSubList = masterServers.subList(i, subListSize);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it'd be non-deterministic at the registry level. (different connections have different registries anyway, so wondering if we need to shuffle again.

public class MasterRegistryFetchException extends HBaseIOException {
public MasterRegistryFetchException(List<ServerName> masters, String failedRPC) {
super(String.format("Exception making rpc %s to masters %s", failedRPC,
masters.stream().map(Objects::toString).collect(Collectors.toList())));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to PrettyPrinter as a general util for any collection.

but this one is less readable than a simple string join using Collections.

Fair point. Switched to good old better performing loops.

@bharathvbharathv left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To summarize the set of changes in the latest force fush.

  • Hedging now happens in the rpc layer and is generic for any call.
  • Implemented MasterRegistry using this hedging rpc framework.
  • Clarified the hedging design in the javadocs.
  • Does not yet implement the adaptable hedging logic, since the patch is already too big. I think it can be done as a follow up.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 32sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 10 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 36sMaven dependency ordering for branch
+1 💚mvninstall5m 17sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile1m 47sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 24sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars4m 33sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 22sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs4m 22sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs6m 13sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for patch
+1 💚mvninstall5m 0sthe patch passed
+1 💚compile1m 45sthe patch passed
+1 💚javac1m 45sthe patch passed
+1 💚checkstyle0m 26shbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5)
+1 💚checkstyle0m 32shbase-client: The patch generated 0 new + 3 unchanged - 6 fixed = 3 total (was 9)
-1 ❌checkstyle1m 23shbase-server: The patch generated 2 new + 227 unchanged - 46 fixed = 229 total (was 273)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
-1 ❌shadedjars2m 28spatch has 10 errors when building our shaded downstream artifacts.
+1 💚hadoopcheck15m 42sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚javadoc1m 21sthe patch passed
+1 💚findbugs6m 30sthe patch passed
_ Other Tests _
+1 💚unit3m 10shbase-common in the patch passed.
+1 💚unit1m 54shbase-client in the patch passed.
-1 ❌unit175m 37shbase-server in the patch failed.
-1 ❌asflicense1m 46sThe patch generated 1 ASF License warnings.
247m 22s
ReasonTests
Failed junit testshadoop.hbase.client.TestFromClientSide
hadoop.hbase.client.TestFromClientSideWithCoprocessor
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux dfed66e1cbf1 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / e41b46c
Default Java1.8.0_181
checkstylehttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/artifact/out/diff-checkstyle-hbase-server.txt
shadedjarshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/artifact/out/patch-shadedjars.txt
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/testReport/
asflicensehttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/artifact/out/patch-asflicense-problems.txt
Max. process+thread count9903 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/3/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 34sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 1sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 10 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 35sMaven dependency ordering for branch
+1 💚mvninstall5m 13sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile1m 44sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 24sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars4m 38sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 19sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs4m 26sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs6m 18sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for patch
+1 💚mvninstall5m 7sthe patch passed
+1 💚compile1m 45sthe patch passed
+1 💚javac1m 45sthe patch passed
+1 💚checkstyle0m 26shbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5)
+1 💚checkstyle0m 31shbase-client: The patch generated 0 new + 3 unchanged - 6 fixed = 3 total (was 9)
+1 💚checkstyle1m 24shbase-server: The patch generated 0 new + 227 unchanged - 46 fixed = 227 total (was 273)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars4m 39spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck15m 44sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚javadoc1m 22sthe patch passed
+1 💚findbugs6m 42sthe patch passed
_ Other Tests _
+1 💚unit3m 11shbase-common in the patch passed.
+1 💚unit1m 53shbase-client in the patch passed.
-1 ❌unit189m 42shbase-server in the patch failed.
+1 💚asflicense1m 45sThe patch does not generate ASF License warnings.
264m 2s
ReasonTests
Failed junit testshadoop.hbase.client.TestFromClientSide
hadoop.hbase.client.TestScannersFromClientSide
hadoop.hbase.client.TestFromClientSideWithCoprocessor
hadoop.hbase.quotas.TestQuotaAdmin
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/4/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux 0e3f574cae6c 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / e41b46c
Default Java1.8.0_181
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/4/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/4/testReport/
Max. process+thread count9909 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/4/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 8sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 10 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 36sMaven dependency ordering for branch
+1 💚mvninstall5m 53sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile1m 43sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 38sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars5m 7sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 17sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs4m 43sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs6m 37sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 14sMaven dependency ordering for patch
+1 💚mvninstall5m 32sthe patch passed
+1 💚compile1m 47sthe patch passed
+1 💚javac1m 47sthe patch passed
+1 💚checkstyle0m 25shbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5)
+1 💚checkstyle0m 35shbase-client: The patch generated 0 new + 3 unchanged - 6 fixed = 3 total (was 9)
+1 💚checkstyle1m 33shbase-server: The patch generated 0 new + 227 unchanged - 46 fixed = 227 total (was 273)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars5m 3spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck17m 21sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚javadoc1m 20sthe patch passed
+1 💚findbugs7m 3sthe patch passed
_ Other Tests _
+1 💚unit3m 1shbase-common in the patch passed.
+1 💚unit1m 50shbase-client in the patch passed.
-1 ❌unit271m 40shbase-server in the patch failed.
+1 💚asflicense1m 10sThe patch does not generate ASF License warnings.
350m 55s
ReasonTests
Failed junit testshadoop.hbase.client.TestFromClientSideWithCoprocessor
hadoop.hbase.quotas.TestClusterScopeQuotaThrottle
hadoop.hbase.client.TestScannersFromClientSide
hadoop.hbase.client.TestFromClientSide
hadoop.hbase.regionserver.TestSplitTransactionOnCluster
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/5/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux f3a8a8df9404 4.15.0-66-generic #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / e41b46c
Default Java1.8.0_181
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/5/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/5/testReport/
Max. process+thread count5491 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/5/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@bharathv

Copy link
Copy Markdown
ContributorAuthor

Fixed all the test failures from the last run in the latest push. One of the fixes comes in as a separate PR to master #963 . To pull that in, the current feature branch needs a rebase on master. Also, from the last test run, the thread usage has substantially dropped due to the test cleanup. I think this is in a good shape for 2nd round of review. Appreciate any feedback.

@asfgit
asfgitforce-pushed the HBASE-18095/client-locate-meta-no-zookeeper branch from e41b46c to 1c41b36CompareDecember 28, 2019 19:24
@bharathv

Copy link
Copy Markdown
ContributorAuthor

@saintstack / @apurtell Any thoughts on the new design? Happy to address any concerns. Thanks.


/**
* Exception thrown when an master registry RPC fails in client. The exception includes the list of
* masters to which RPC was attempted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this include a throwable associated with each master? They can be failing for different reasons.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They will be logged anyway, didn't want to include them here because it will be too verbose. Thoughts?

if (conn.getLastTouched() < closeBeforeTime && !conn.isActive()) {
if (LOG.isTraceEnabled()) LOG.trace("Cleanup idle connection to " + conn.remoteId().address);
if (LOG.isTraceEnabled()) {
LOG.trace("Cleanup idle connection to " + conn.remoteId().address);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use format string.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Configured list of masters to probe the meta information from.
private final List<ServerName> masterServers;
// Controls the fan out of the hedged requests. Requests are made in batches of this number until
// all the servers are exhausted. The first returned result is passed back to the client.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infact it is multiple instances per connection (look at the callers of AsyncRegistryFactory#getRegistry()). I think there is definitely a scope for cleanup here. Can i revisit this as a follow up?

This is the state of things before you arrived, right? I'd say file this as a separate ticket, as another cleanup project for rebase, or for after this code lands. I think I'd prefer to see the former, but it's not a strong preference ATM. I do consider it a blocker for back port to a branch-2, however.

class HedgedRpcChannel implements RpcChannel {
private static final Logger LOG = LoggerFactory.getLogger(HedgedRpcChannel.class);

private final AbstractRpcClient rpcClient;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This instance variable needs to be templatized. Probably <?> is sufficient.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* @param result Result to be set.
*/
public void setResultIfNotSet(Message result, HBaseRpcController rpcController) {
if (result == null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd that the null check is vs result but it's the value of rpcController.getFailed() that is actually used. I would expect the null check against the value of getFailed. There would need to be a subsequent null check against result to check for a should-not-happen kind of scenario.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

// Cancel all pending in flight calls.
for (Call call: callsInFlight) {
// It is ok to do it for all calls as it is a no-op if the call is already done.
call.setException(new CallCancelledException("Hedged call succeeded."));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps something more descriptive like "Canceled because sibling hedged call succeeded". It's odd to see the message explaining an exception describe a successful result.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

public void testHedgedAsyncTimeouts() throws Exception {
List<RpcServer> rpcServers = new ArrayList<>();
List<InetSocketAddress> addresses = new ArrayList<>();
// Create a mix of running and failing servers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stale comment? I only see running servers in this test.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

public static final class HMasterForTest extends HMaster {

public HMasterForTest(Configuration conf) throws IOException, KeeperException {
public HMasterForTest(Configuration conf) throws IOException {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your second miracle.

}

@Test
public void testMetaRegionState() throws Exception {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@asfgit
asfgitforce-pushed the HBASE-18095/client-locate-meta-no-zookeeper branch from 1c41b36 to dffa9beCompareJanuary 3, 2020 00:21
@ndimiduk

Copy link
Copy Markdown
Member

FYI, I rebased the feature branch onto the tip of master.

@bharathvbharathv left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndimiduk Thanks for taking your time to review this. You see a ton of already-reviewed-changes here because of the force pushes to the feature branch. You shouldn't be seeing them once I rebase the patch. Will address your comments after the rebase.

@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 12sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 31 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 33sMaven dependency ordering for branch
+1 💚mvninstall5m 50sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile1m 45sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 36sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars5m 8sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 24sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs5m 17sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs7m 21sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 14sMaven dependency ordering for patch
+1 💚mvninstall5m 37sthe patch passed
+1 💚compile1m 44sthe patch passed
+1 💚javac1m 44sthe patch passed
+1 💚checkstyle0m 26shbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5)
-1 ❌checkstyle0m 33shbase-client: The patch generated 2 new + 14 unchanged - 10 fixed = 16 total (was 24)
+1 💚checkstyle1m 31shbase-server: The patch generated 0 new + 226 unchanged - 48 fixed = 226 total (was 274)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars5m 1spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck17m 33sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚javadoc1m 18sthe patch passed
+1 💚findbugs7m 19sthe patch passed
_ Other Tests _
+1 💚unit3m 12shbase-common in the patch passed.
+1 💚unit1m 54shbase-client in the patch passed.
-1 ❌unit171m 59shbase-server in the patch failed.
+1 💚asflicense1m 16sThe patch does not generate ASF License warnings.
252m 59s
ReasonTests
Failed junit testshadoop.hbase.client.TestScannersFromClientSide
hadoop.hbase.client.TestFromClientSide
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/7/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux 265a7760b785 4.15.0-66-generic #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / dffa9be
Default Java1.8.0_181
checkstylehttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/7/artifact/out/diff-checkstyle-hbase-client.txt
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/7/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/7/testReport/
Max. process+thread count6669 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/7/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@bharathv
bharathv requested a review from apurtellJanuary 3, 2020 21:10
@Apache-HBase

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 20sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 31 new or modified test files.
_ HBASE-18095/client-locate-meta-no-zookeeper Compile Tests _
+0 🆗mvndep0m 36sMaven dependency ordering for branch
+1 💚mvninstall5m 59sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚compile2m 3sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚checkstyle2m 39sHBASE-18095/client-locate-meta-no-zookeeper passed
+1 💚shadedjars5m 15sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 18sHBASE-18095/client-locate-meta-no-zookeeper passed
+0 🆗spotbugs4m 43sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs6m 35sHBASE-18095/client-locate-meta-no-zookeeper passed
_ Patch Compile Tests _
+0 🆗mvndep0m 14sMaven dependency ordering for patch
+1 💚mvninstall5m 34sthe patch passed
+1 💚compile1m 45sthe patch passed
+1 💚javac1m 45sthe patch passed
+1 💚checkstyle0m 26shbase-common: The patch generated 0 new + 4 unchanged - 1 fixed = 4 total (was 5)
-1 ❌checkstyle0m 33shbase-client: The patch generated 2 new + 14 unchanged - 10 fixed = 16 total (was 24)
+1 💚checkstyle1m 34shbase-server: The patch generated 0 new + 224 unchanged - 50 fixed = 224 total (was 274)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars5m 5spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck17m 34sPatch does not cause any errors with Hadoop 2.8.5 2.9.2 or 3.1.2.
+1 💚javadoc1m 16sthe patch passed
+1 💚findbugs7m 33sthe patch passed
_ Other Tests _
+1 💚unit3m 7shbase-common in the patch passed.
+1 💚unit1m 55shbase-client in the patch passed.
-1 ❌unit166m 9shbase-server in the patch failed.
+1 💚asflicense1m 13sThe patch does not generate ASF License warnings.
247m 11s
ReasonTests
Failed junit testshadoop.hbase.client.TestFromClientSideWithCoprocessor
hadoop.hbase.client.TestFromClientSide
SubsystemReport/Notes
DockerClient=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/8/artifact/out/Dockerfile
GITHUB PR#954
JIRA IssueHBASE-23305
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux 1bcc84663300 4.15.0-66-generic #75-Ubuntu SMP Tue Oct 1 05:24:09 UTC 2019 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-954/out/precommit/personality/provided.sh
git revisionHBASE-18095/client-locate-meta-no-zookeeper / dffa9be
Default Java1.8.0_181
checkstylehttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/8/artifact/out/diff-checkstyle-hbase-client.txt
unithttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/8/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/8/testReport/
Max. process+thread count6395 (vs. ulimit of 10000)
modulesC: hbase-common hbase-client hbase-server U: .
Console outputhttps://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-954/8/console
versionsgit=2.11.0 maven=2018-06-17T18:33:14Z) findbugs=3.1.11
Powered byApache Yetus 0.11.1 https://yetus.apache.org

This message was automatically generated.

@bharathv

Copy link
Copy Markdown
ContributorAuthor

Fixed the check style issues and rebased on the latest tip of the feature branch (and squashed all the commits). Test failures seem to be flakes and run fine for me locally. I think this is good to go for another round of review.

@bharathv
bharathv requested a review from ndimidukJanuary 4, 2020 02:42
@ndimiduk
ndimiduk merged commit 4a74bb3 into apache:HBASE-18095/client-locate-meta-no-zookeeperJan 14, 2020
asfgit pushed a commit that referenced this pull request Jan 14, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request Jan 21, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
ndimiduk pushed a commit to ndimiduk/hbase that referenced this pull request Jan 21, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Jan 24, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Jan 28, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
(cherry picked from commit 62da419)
asfgit pushed a commit that referenced this pull request Jan 29, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Jan 29, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Jan 30, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 2, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 2, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Feb 3, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Feb 4, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Feb 5, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
asfgit pushed a commit that referenced this pull request Feb 5, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit that referenced this pull request Feb 9, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit that referenced this pull request Feb 11, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit that referenced this pull request Feb 13, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 14, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 17, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit that referenced this pull request Feb 18, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 20, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit that referenced this pull request Feb 20, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 23, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
(cherry picked from commit 62da419)
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 25, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
(cherry picked from commit 62da419)
bharathv added a commit to bharathv/hbase that referenced this pull request Feb 26, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
(cherry picked from commit 62da419)
bharathv added a commit that referenced this pull request Feb 27, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
(cherry picked from commit 62da419)
thangTang pushed a commit to thangTang/hbase that referenced this pull request Apr 16, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
thangTang pushed a commit to thangTang/hbase that referenced this pull request Apr 16, 2020
Implements a master based registry for clients.
- Supports hedged RPCs (fan out configured via configs).
- Parameterized existing client tests to run with multiple registry combinations.
- Added unit-test coverage for the new registry implementation.
Signed-off-by: Nick Dimiduk <ndimiduk@apache.org>
Signed-off-by: stack <stack@apache.org>
Signed-off-by: Andrew Purtell <apurtell@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@bharathv@Apache-HBase@ndimiduk@saintstack@apurtell