You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/05/21 18:32:34 UTC
[GitHub] [hbase] sguggilam opened a new pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
sguggilam opened a new pull request #1755:
URL: https://github.com/apache/hbase/pull/1755
…region close requests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r430767349
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1972,6 +1975,13 @@ private void unassign(final HRegionInfo region,
final RegionState state, final int versionOfClosingNode,
final ServerName dest, final boolean transitionInZK,
final ServerName src) {
+ String encodedName = region.getEncodedName();
+ AtomicInteger failedCloseCount = failedCloseTracker.get(encodedName);
+ if (failedCloseCount == null) {
+ failedCloseCount = new AtomicInteger();
+ failedCloseTracker.put(encodedName, failedCloseCount);
Review comment:
Aren't all the codepaths reaching this point, expected to take an exclusive lock on the region.encodedName()? If so, wondering if we should worry about the non-thread-safe access for this map. I checked all the callers, all except one path in forceRegionStateToOffline() follow this pattern, we should probably fix that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1997,80 +2007,76 @@ private void unassign(final HRegionInfo region,
}
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
} catch (Throwable t) {
long sleepTime = 0;
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
// RS is aborting or stopping, we cannot offline the region since the region may need
- // to do WAL recovery. Until we see the RS expiration, we should retry.
+ // to do WAL recovery. Until we see the RS expiration, we should retry.
sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
} else if (t instanceof NotServingRegionException) {
- LOG.debug("Offline " + region.getRegionNameAsString()
- + ", it's not any more on " + server, t);
+ LOG.debug(
+ "Offline " + region.getRegionNameAsString() + ", it's not any more on " + server, t);
if (transitionInZK) {
deleteClosingOrClosedNode(region, server);
}
if (state != null) {
regionOffline(region);
}
return;
- } else if ((t instanceof FailedServerException) || (state != null &&
- t instanceof RegionAlreadyInTransitionException)) {
- if (t instanceof FailedServerException) {
- sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
+ } else if ((t instanceof FailedServerException)
Review comment:
Is there any change in functionality of this section of diff? I think the answer is no and its mostly indents, but I wanted to double check..can you please confirm?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
Review comment:
Pardon my ignorance but I don't fully understand the fix. I think the ask in the jira to spread out the unassigns a bit by using a backoff based approach. To do that why not just fix the sleepTime above in L2075 to use an exponential backoff based approach?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
+ getFailedAttempts(encodedName, failedCloseTracker));
+ if (failedCloseCount.incrementAndGet() <= maximumAttempts && sleepTime > 0) {
+ if (failedCloseTracker.containsKey(encodedName)) {
+ // Sleep before trying unassign if this region has failed to close before
+ scheduledThreadPoolExecutor.schedule(new DelayedUnAssignCallable(this, region, state,
Review comment:
javadoc for the method says "Send CLOSE RPC if the server is online, otherwise, offline the region.". With this delayed callables, aren't we violating that? If we hit this block, we will still be in the CLOSING state with the thread pool retrying in the background. Curious if that causes any issues in the statemachine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r432192279
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
Review comment:
Right, so my point is this. If there is an exception in sendRegionClose() (say ServerTooBusy), we enter the catch() block in L2024, we don't match the if / else if in L2031 (failed server) / 2038 (region not serving) and that leaves us with just looping for "maximumAttempts" quickly in succession without a backoff (this is the problem we are trying to address). My point was why not do something like this.
```
if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
......
} else if ((t instanceof FailedServerException)....) {
.....
} else {
// Handle any other remote exception
// use your exponential backoff algorithm...
}
```
Basically we have the loop for maxAttempts just that it is not using an exponential backoff. Just plug it in an else {} block and we are good? What is advantage of doing it with a delayed callable, doesn't that just complicate the state machine with async requests and book-keeping or is there something that I'm missing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-636408973
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 6m 58s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 9m 45s | branch-1 passed |
| +1 :green_heart: | compile | 0m 40s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | compile | 0m 43s | branch-1 passed with JDK v1.7.0_262 |
| +1 :green_heart: | checkstyle | 1m 48s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 1s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 48s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 42s | branch-1 passed with JDK v1.7.0_262 |
| +0 :ok: | spotbugs | 2m 59s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 56s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 52s | the patch passed |
| +1 :green_heart: | compile | 0m 41s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javac | 0m 41s | the patch passed |
| +1 :green_heart: | compile | 0m 42s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | javac | 0m 42s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 37s | hbase-server: The patch generated 0 new + 245 unchanged - 36 fixed = 245 total (was 281) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 2m 50s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 4m 36s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 31s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 42s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | findbugs | 2m 51s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 132m 20s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 37s | The patch does not generate ASF License warnings. |
| | | 180m 5s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hbase.procedure.TestFailedProcCleanup |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.10 Server=19.03.10 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1755 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 6acd71b1f34a 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1755/out/precommit/personality/provided.sh |
| git revision | branch-1 / 61fa21e |
| Default Java | 1.7.0_262 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:1.8.0_252 /usr/lib/jvm/zulu-7-amd64:1.7.0_262 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/4/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/4/testReport/ |
| Max. process+thread count | 4586 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/4/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-636253821
@bharathv pushed the latest changes incorporating review comments, please check
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-632363276
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 7m 2s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 8m 41s | branch-1 passed |
| +1 :green_heart: | compile | 0m 39s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | compile | 0m 45s | branch-1 passed with JDK v1.7.0_262 |
| +1 :green_heart: | checkstyle | 1m 43s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 1s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 40s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 39s | branch-1 passed with JDK v1.7.0_262 |
| +0 :ok: | spotbugs | 2m 53s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 51s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 55s | the patch passed |
| +1 :green_heart: | compile | 0m 42s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javac | 0m 42s | the patch passed |
| +1 :green_heart: | compile | 0m 44s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | javac | 0m 44s | the patch passed |
| -1 :x: | checkstyle | 1m 39s | hbase-server: The patch generated 17 new + 279 unchanged - 2 fixed = 296 total (was 281) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 3m 32s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 6m 12s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 37s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 53s | the patch passed with JDK v1.7.0_262 |
| -1 :x: | findbugs | 4m 8s | hbase-server generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
||| _ Other Tests _ |
| -1 :x: | unit | 139m 18s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 33s | The patch does not generate ASF License warnings. |
| | | 190m 31s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:hbase-server |
| | Sequence of calls to java.util.concurrent.ConcurrentHashMap may not be atomic in org.apache.hadoop.hbase.master.AssignmentManager.unassign(HRegionInfo, RegionState, int, ServerName, boolean, ServerName) At AssignmentManager.java:may not be atomic in org.apache.hadoop.hbase.master.AssignmentManager.unassign(HRegionInfo, RegionState, int, ServerName, boolean, ServerName) At AssignmentManager.java:[line 1982] |
| Failed junit tests | hadoop.hbase.client.TestAdmin2 |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1755 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 854dba18eabe 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1755/out/precommit/personality/provided.sh |
| git revision | branch-1 / 3235b56 |
| Default Java | 1.7.0_262 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:1.8.0_252 /usr/lib/jvm/zulu-7-amd64:1.7.0_262 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/diff-checkstyle-hbase-server.txt |
| findbugs | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/new-findbugs-hbase-server.html |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/testReport/ |
| Max. process+thread count | 4654 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-636119423
@apurtell @bharathv Incorporated the code review comments, please take a look
Also the Assignment Manager V2 already have the retry with back off logic for failed open/close requests (https://github.com/apache/hbase/blob/branch-2/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java#L360) , so the change is not needed for branch-2 and above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-636274875
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 8m 26s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 10m 38s | branch-1 passed |
| +1 :green_heart: | compile | 0m 55s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | compile | 1m 4s | branch-1 passed with JDK v1.7.0_262 |
| +1 :green_heart: | checkstyle | 2m 25s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 50s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 56s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 52s | branch-1 passed with JDK v1.7.0_262 |
| +0 :ok: | spotbugs | 3m 42s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 3m 39s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 29s | the patch passed |
| +1 :green_heart: | compile | 0m 54s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javac | 0m 54s | the patch passed |
| +1 :green_heart: | compile | 1m 5s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | javac | 1m 5s | the patch passed |
| -1 :x: | checkstyle | 2m 20s | hbase-server: The patch generated 18 new + 245 unchanged - 36 fixed = 263 total (was 281) |
| -1 :x: | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | shadedjars | 4m 0s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 5m 58s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 39s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 53s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | findbugs | 3m 54s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 131m 47s | hbase-server in the patch passed. |
| +1 :green_heart: | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
| | | 192m 1s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.10 Server=19.03.10 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1755 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 256d68c4cf8b 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1755/out/precommit/personality/provided.sh |
| git revision | branch-1 / 61fa21e |
| Default Java | 1.7.0_262 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:1.8.0_252 /usr/lib/jvm/zulu-7-amd64:1.7.0_262 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/3/artifact/out/diff-checkstyle-hbase-server.txt |
| whitespace | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/3/artifact/out/whitespace-eol.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/3/testReport/ |
| Max. process+thread count | 4653 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/3/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r432203290
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
Review comment:
There are two reasons for this:
1. It also deals with the case where the sendRegionClose() itself returns false without getting into the catch block though I am not very sure of the case where it would reach there. the method doc says the method returns false if it doesn't acknowledge the request
2. The exponential backoff is configurable and can even be 5 minutes , so there is no point holding the thread for that amount of time and rather have it deal asynchronously.
I cannot think of any state machine issue as even if just sleep the current thread , the state would remain the same and the method as such is not returning anything and anyone else would just check the state change made by the method either synchronously or asynchronously. Rather we would delegate it to a new thread and not block the current thread.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv merged pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
bharathv merged pull request #1755:
URL: https://github.com/apache/hbase/pull/1755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-636193000
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 35s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 9m 55s | branch-1 passed |
| +1 :green_heart: | compile | 0m 40s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | compile | 0m 45s | branch-1 passed with JDK v1.7.0_262 |
| +1 :green_heart: | checkstyle | 1m 51s | branch-1 passed |
| +1 :green_heart: | shadedjars | 2m 58s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 47s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 41s | branch-1 passed with JDK v1.7.0_262 |
| +0 :ok: | spotbugs | 3m 2s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 59s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 53s | the patch passed |
| +1 :green_heart: | compile | 0m 38s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javac | 0m 38s | the patch passed |
| +1 :green_heart: | compile | 0m 45s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | javac | 0m 45s | the patch passed |
| -1 :x: | checkstyle | 1m 40s | hbase-server: The patch generated 18 new + 245 unchanged - 36 fixed = 263 total (was 281) |
| -1 :x: | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | shadedjars | 2m 47s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 4m 35s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 31s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 40s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | findbugs | 2m 53s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 121m 25s | hbase-server in the patch passed. |
| +1 :green_heart: | asflicense | 0m 37s | The patch does not generate ASF License warnings. |
| | | 163m 10s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.10 Server=19.03.10 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1755 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 23c9fe6d5a60 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1755/out/precommit/personality/provided.sh |
| git revision | branch-1 / 61fa21e |
| Default Java | 1.7.0_262 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:1.8.0_252 /usr/lib/jvm/zulu-7-amd64:1.7.0_262 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/2/artifact/out/diff-checkstyle-hbase-server.txt |
| whitespace | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/2/artifact/out/whitespace-eol.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/2/testReport/ |
| Max. process+thread count | 4503 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/2/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r431429535
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1972,6 +1975,13 @@ private void unassign(final HRegionInfo region,
final RegionState state, final int versionOfClosingNode,
final ServerName dest, final boolean transitionInZK,
final ServerName src) {
+ String encodedName = region.getEncodedName();
+ AtomicInteger failedCloseCount = failedCloseTracker.get(encodedName);
+ if (failedCloseCount == null) {
+ failedCloseCount = new AtomicInteger();
+ failedCloseTracker.put(encodedName, failedCloseCount);
Review comment:
That's fine
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r432203290
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
Review comment:
There are two reasons for this:
1. It also deals with the case where the sendRegionClose() itself returns false without getting into the catch block though I am not very sure of the case where it would reach there. the method doc says the method returns false if it doesn't acknowledge the request
2. The current sleepTime in the else of catch block is static at 100ms. The exponential backoff is configurable and can even be 5 minutes , so there is no point holding the thread for that amount of time and rather have it deal asynchronously.
I cannot think of any state machine issue as even if just sleep the current thread , the state would remain the same and the method as such is not returning anything and anyone else would just check the state change made by the method either synchronously or asynchronously. Rather we would delegate it to a new thread and not block the current thread.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-632969485
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 6m 52s | 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 1 new or modified test files. |
||| _ branch-1 Compile Tests _ |
| +1 :green_heart: | mvninstall | 9m 48s | branch-1 passed |
| +1 :green_heart: | compile | 0m 41s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | compile | 0m 44s | branch-1 passed with JDK v1.7.0_262 |
| +1 :green_heart: | checkstyle | 1m 46s | branch-1 passed |
| +1 :green_heart: | shadedjars | 3m 1s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 49s | branch-1 passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 42s | branch-1 passed with JDK v1.7.0_262 |
| +0 :ok: | spotbugs | 3m 1s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 58s | branch-1 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 56s | the patch passed |
| +1 :green_heart: | compile | 0m 41s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javac | 0m 41s | the patch passed |
| +1 :green_heart: | compile | 0m 46s | the patch passed with JDK v1.7.0_262 |
| +1 :green_heart: | javac | 0m 46s | the patch passed |
| -1 :x: | checkstyle | 1m 36s | hbase-server: The patch generated 17 new + 279 unchanged - 2 fixed = 296 total (was 281) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 2m 50s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 4m 30s | Patch does not cause any errors with Hadoop 2.8.5 2.9.2. |
| +1 :green_heart: | javadoc | 0m 29s | the patch passed with JDK v1.8.0_252 |
| +1 :green_heart: | javadoc | 0m 41s | the patch passed with JDK v1.7.0_262 |
| -1 :x: | findbugs | 2m 52s | hbase-server generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
||| _ Other Tests _ |
| -1 :x: | unit | 128m 13s | hbase-server in the patch failed. |
| +1 :green_heart: | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
| | | 175m 54s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:hbase-server |
| | Sequence of calls to java.util.concurrent.ConcurrentHashMap may not be atomic in org.apache.hadoop.hbase.master.AssignmentManager.unassign(HRegionInfo, RegionState, int, ServerName, boolean, ServerName) At AssignmentManager.java:may not be atomic in org.apache.hadoop.hbase.master.AssignmentManager.unassign(HRegionInfo, RegionState, int, ServerName, boolean, ServerName) At AssignmentManager.java:[line 1982] |
| Failed junit tests | hadoop.hbase.client.TestClientScannerRPCTimeout |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/1755 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 01f251a87674 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/Base-PreCommit-GitHub-PR_PR-1755/out/precommit/personality/provided.sh |
| git revision | branch-1 / 3235b56 |
| Default Java | 1.7.0_262 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:1.8.0_252 /usr/lib/jvm/zulu-7-amd64:1.7.0_262 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/diff-checkstyle-hbase-server.txt |
| findbugs | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/new-findbugs-hbase-server.html |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/artifact/out/patch-unit-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/testReport/ |
| Max. process+thread count | 3874 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1755/1/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r430708920
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1972,6 +1975,13 @@ private void unassign(final HRegionInfo region,
final RegionState state, final int versionOfClosingNode,
final ServerName dest, final boolean transitionInZK,
final ServerName src) {
+ String encodedName = region.getEncodedName();
+ AtomicInteger failedCloseCount = failedCloseTracker.get(encodedName);
+ if (failedCloseCount == null) {
+ failedCloseCount = new AtomicInteger();
+ failedCloseTracker.put(encodedName, failedCloseCount);
Review comment:
We can race between test and put, potentially undercounting (by ref overwrite). Probably should test if the return of putIfAbsent is not null, and use/update that. If the code that updates `failedOpenTracker` also has this racy pattern it should be fixed too.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1972,6 +1975,13 @@ private void unassign(final HRegionInfo region,
final RegionState state, final int versionOfClosingNode,
final ServerName dest, final boolean transitionInZK,
final ServerName src) {
+ String encodedName = region.getEncodedName();
+ AtomicInteger failedCloseCount = failedCloseTracker.get(encodedName);
+ if (failedCloseCount == null) {
+ failedCloseCount = new AtomicInteger();
+ failedCloseTracker.put(encodedName, failedCloseCount);
Review comment:
We can race between test and put, potentially undercounting (by ref overwrite). Test if the return of putIfAbsent is not null, and use/update that. It will either be the new atomic inserted via putIfAbsent or an existing atomic. _Then_ increment. If the code that updates `failedOpenTracker` also has this racy pattern it should be fixed too.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1972,6 +1975,13 @@ private void unassign(final HRegionInfo region,
final RegionState state, final int versionOfClosingNode,
final ServerName dest, final boolean transitionInZK,
final ServerName src) {
+ String encodedName = region.getEncodedName();
+ AtomicInteger failedCloseCount = failedCloseTracker.get(encodedName);
+ if (failedCloseCount == null) {
+ failedCloseCount = new AtomicInteger();
+ failedCloseTracker.put(encodedName, failedCloseCount);
Review comment:
We can race between test and put, potentially undercounting (by ref overwrite). Test if the return of putIfAbsent is not null. If not null, use that instead of the atomic instance you just created. _Then_ increment. If the code that updates `failedOpenTracker` also has this racy pattern it should be fixed too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r432207528
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
Review comment:
The current code does the sleep in the current thread where it is handling the region already in transition kind of case where there is a max wait time and hence they just sleep for every 100ms and keep retrying before the max ttime ends. In our case we definitely know that we don't want to retry before a configured backoff time, so we can as well schedule it 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
[GitHub] [hbase] bharathv commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r432774809
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1995,97 +1995,100 @@ private void unassign(final HRegionInfo region,
}
return;
}
+ long sleepTime = 0;
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
- } catch (Throwable t) {
- long sleepTime = 0;
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
+ } catch (Throwable t) {
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
Review comment:
nit: (t instanceof RegionServerAbortedException) is redundant (per static analysis check in the IDE)...
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java
##########
@@ -598,6 +601,68 @@ public void testCloseFailed() throws Exception {
}
}
+ /**
+ * This tests region close with exponential backoff
+ */
+ @Test(timeout = 60000)
+ public void testCloseRegionWithExponentialBackOff() throws Exception {
+ String table = "testCloseRegionWithExponentialBackOff";
Review comment:
nit: We use TestTableName.getTableName() that automatically gets the TableName with the running test name.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1995,97 +1995,100 @@ private void unassign(final HRegionInfo region,
}
return;
}
+ long sleepTime = 0;
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
- } catch (Throwable t) {
- long sleepTime = 0;
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
+ } catch (Throwable t) {
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
// RS is aborting or stopping, we cannot offline the region since the region may need
- // to do WAL recovery. Until we see the RS expiration, we should retry.
+ // to do WAL recovery. Until we see the RS expiration, we should retry.
sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
} else if (t instanceof NotServingRegionException) {
- LOG.debug("Offline " + region.getRegionNameAsString()
- + ", it's not any more on " + server, t);
+ LOG.debug(
+ "Offline " + region.getRegionNameAsString() + ", it's not any more on " + server, t);
if (transitionInZK) {
deleteClosingOrClosedNode(region, server);
}
if (state != null) {
regionOffline(region);
}
return;
- } else if ((t instanceof FailedServerException) || (state != null &&
- t instanceof RegionAlreadyInTransitionException)) {
- if (t instanceof FailedServerException) {
- sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
+ } else if ((t instanceof FailedServerException)
+ || (state != null && t instanceof RegionAlreadyInTransitionException)) {
+ if (t instanceof FailedServerException) {
+ sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
- } else {
- // RS is already processing this region, only need to update the timestamp
- LOG.debug("update " + state + " the timestamp.");
- state.updateTimestampToNow();
- if (maxWaitTime < 0) {
- maxWaitTime =
- EnvironmentEdgeManager.currentTime()
- + conf.getLong(ALREADY_IN_TRANSITION_WAITTIME,
- DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
- }
- long now = EnvironmentEdgeManager.currentTime();
- if (now < maxWaitTime) {
- LOG.debug("Region is already in transition; "
- + "waiting up to " + (maxWaitTime - now) + "ms", t);
- sleepTime = 100;
- i--; // reset the try count
- logRetries = false;
+ } else {
+ // RS is already processing this region, only need to update the timestamp
+ LOG.debug("update " + state + " the timestamp.");
+ state.updateTimestampToNow();
+ if (maxWaitTime < 0) {
+ maxWaitTime = EnvironmentEdgeManager.currentTime() + conf.getLong(
+ ALREADY_IN_TRANSITION_WAITTIME, DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
+ }
+ long now = EnvironmentEdgeManager.currentTime();
+ if (now < maxWaitTime) {
+ LOG.debug("Region is already in transition; " + "waiting up to "
+ + (maxWaitTime - now) + "ms",
+ t);
+ sleepTime = 100;
+ i--; // reset the try count
+ logRetries = false;
+ }
+ }
}
- }
- }
-
- try {
- if (sleepTime > 0) {
- Thread.sleep(sleepTime);
- }
- } catch (InterruptedException ie) {
- LOG.warn("Failed to unassign "
- + region.getRegionNameAsString() + " since interrupted", ie);
- Thread.currentThread().interrupt();
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
- }
- return;
- }
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
+ // If sleepTime is not set by any of the cases, set it to sleep for
Review comment:
nit: Can you please add a few comments on the code flow so that its easier for other to follow?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1995,97 +1995,100 @@ private void unassign(final HRegionInfo region,
}
return;
}
+ long sleepTime = 0;
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
- } catch (Throwable t) {
- long sleepTime = 0;
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
+ } catch (Throwable t) {
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
// RS is aborting or stopping, we cannot offline the region since the region may need
- // to do WAL recovery. Until we see the RS expiration, we should retry.
+ // to do WAL recovery. Until we see the RS expiration, we should retry.
sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
} else if (t instanceof NotServingRegionException) {
- LOG.debug("Offline " + region.getRegionNameAsString()
- + ", it's not any more on " + server, t);
+ LOG.debug(
+ "Offline " + region.getRegionNameAsString() + ", it's not any more on " + server, t);
if (transitionInZK) {
deleteClosingOrClosedNode(region, server);
}
if (state != null) {
regionOffline(region);
}
return;
- } else if ((t instanceof FailedServerException) || (state != null &&
- t instanceof RegionAlreadyInTransitionException)) {
- if (t instanceof FailedServerException) {
- sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
+ } else if ((t instanceof FailedServerException)
+ || (state != null && t instanceof RegionAlreadyInTransitionException)) {
+ if (t instanceof FailedServerException) {
+ sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
- } else {
- // RS is already processing this region, only need to update the timestamp
- LOG.debug("update " + state + " the timestamp.");
- state.updateTimestampToNow();
- if (maxWaitTime < 0) {
- maxWaitTime =
- EnvironmentEdgeManager.currentTime()
- + conf.getLong(ALREADY_IN_TRANSITION_WAITTIME,
- DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
- }
- long now = EnvironmentEdgeManager.currentTime();
- if (now < maxWaitTime) {
- LOG.debug("Region is already in transition; "
- + "waiting up to " + (maxWaitTime - now) + "ms", t);
- sleepTime = 100;
- i--; // reset the try count
- logRetries = false;
+ } else {
+ // RS is already processing this region, only need to update the timestamp
+ LOG.debug("update " + state + " the timestamp.");
+ state.updateTimestampToNow();
+ if (maxWaitTime < 0) {
+ maxWaitTime = EnvironmentEdgeManager.currentTime() + conf.getLong(
+ ALREADY_IN_TRANSITION_WAITTIME, DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
+ }
+ long now = EnvironmentEdgeManager.currentTime();
+ if (now < maxWaitTime) {
+ LOG.debug("Region is already in transition; " + "waiting up to "
+ + (maxWaitTime - now) + "ms",
+ t);
+ sleepTime = 100;
+ i--; // reset the try count
+ logRetries = false;
+ }
+ }
}
- }
- }
-
- try {
- if (sleepTime > 0) {
- Thread.sleep(sleepTime);
- }
- } catch (InterruptedException ie) {
- LOG.warn("Failed to unassign "
- + region.getRegionNameAsString() + " since interrupted", ie);
- Thread.currentThread().interrupt();
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
- }
- return;
- }
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
Review comment:
merge this logging and the logging in L2074 and log towards the end?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1995,97 +1995,100 @@ private void unassign(final HRegionInfo region,
}
return;
}
+ long sleepTime = 0;
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
- } catch (Throwable t) {
- long sleepTime = 0;
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
+ } catch (Throwable t) {
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
// RS is aborting or stopping, we cannot offline the region since the region may need
- // to do WAL recovery. Until we see the RS expiration, we should retry.
+ // to do WAL recovery. Until we see the RS expiration, we should retry.
sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
} else if (t instanceof NotServingRegionException) {
- LOG.debug("Offline " + region.getRegionNameAsString()
- + ", it's not any more on " + server, t);
+ LOG.debug(
+ "Offline " + region.getRegionNameAsString() + ", it's not any more on " + server, t);
if (transitionInZK) {
deleteClosingOrClosedNode(region, server);
}
if (state != null) {
regionOffline(region);
}
return;
- } else if ((t instanceof FailedServerException) || (state != null &&
- t instanceof RegionAlreadyInTransitionException)) {
- if (t instanceof FailedServerException) {
- sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
+ } else if ((t instanceof FailedServerException)
+ || (state != null && t instanceof RegionAlreadyInTransitionException)) {
+ if (t instanceof FailedServerException) {
+ sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
- } else {
- // RS is already processing this region, only need to update the timestamp
- LOG.debug("update " + state + " the timestamp.");
- state.updateTimestampToNow();
- if (maxWaitTime < 0) {
- maxWaitTime =
- EnvironmentEdgeManager.currentTime()
- + conf.getLong(ALREADY_IN_TRANSITION_WAITTIME,
- DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
- }
- long now = EnvironmentEdgeManager.currentTime();
- if (now < maxWaitTime) {
- LOG.debug("Region is already in transition; "
- + "waiting up to " + (maxWaitTime - now) + "ms", t);
- sleepTime = 100;
- i--; // reset the try count
- logRetries = false;
+ } else {
+ // RS is already processing this region, only need to update the timestamp
+ LOG.debug("update " + state + " the timestamp.");
+ state.updateTimestampToNow();
+ if (maxWaitTime < 0) {
+ maxWaitTime = EnvironmentEdgeManager.currentTime() + conf.getLong(
+ ALREADY_IN_TRANSITION_WAITTIME, DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
+ }
+ long now = EnvironmentEdgeManager.currentTime();
+ if (now < maxWaitTime) {
+ LOG.debug("Region is already in transition; " + "waiting up to "
+ + (maxWaitTime - now) + "ms",
+ t);
+ sleepTime = 100;
+ i--; // reset the try count
+ logRetries = false;
+ }
+ }
}
- }
- }
-
- try {
- if (sleepTime > 0) {
- Thread.sleep(sleepTime);
- }
- } catch (InterruptedException ie) {
- LOG.warn("Failed to unassign "
- + region.getRegionNameAsString() + " since interrupted", ie);
- Thread.currentThread().interrupt();
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
- }
- return;
- }
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
+ // If sleepTime is not set by any of the cases, set it to sleep for
+ // configured exponential backoff time
+ if (sleepTime == 0 && i != maximumAttempts) {
Review comment:
How about a more readable condition here?
if (regionCloseFailed || anyOtherExceptionThrown) { // set these flags above
}
I think that makes the flow easier to understand.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1995,97 +1995,100 @@ private void unassign(final HRegionInfo region,
}
return;
}
+ long sleepTime = 0;
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
- } catch (Throwable t) {
- long sleepTime = 0;
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
+ } catch (Throwable t) {
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
// RS is aborting or stopping, we cannot offline the region since the region may need
- // to do WAL recovery. Until we see the RS expiration, we should retry.
+ // to do WAL recovery. Until we see the RS expiration, we should retry.
sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
} else if (t instanceof NotServingRegionException) {
- LOG.debug("Offline " + region.getRegionNameAsString()
- + ", it's not any more on " + server, t);
+ LOG.debug(
+ "Offline " + region.getRegionNameAsString() + ", it's not any more on " + server, t);
if (transitionInZK) {
deleteClosingOrClosedNode(region, server);
}
if (state != null) {
regionOffline(region);
}
return;
- } else if ((t instanceof FailedServerException) || (state != null &&
- t instanceof RegionAlreadyInTransitionException)) {
- if (t instanceof FailedServerException) {
- sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
+ } else if ((t instanceof FailedServerException)
+ || (state != null && t instanceof RegionAlreadyInTransitionException)) {
+ if (t instanceof FailedServerException) {
+ sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
- } else {
- // RS is already processing this region, only need to update the timestamp
- LOG.debug("update " + state + " the timestamp.");
- state.updateTimestampToNow();
- if (maxWaitTime < 0) {
- maxWaitTime =
- EnvironmentEdgeManager.currentTime()
- + conf.getLong(ALREADY_IN_TRANSITION_WAITTIME,
- DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
- }
- long now = EnvironmentEdgeManager.currentTime();
- if (now < maxWaitTime) {
- LOG.debug("Region is already in transition; "
- + "waiting up to " + (maxWaitTime - now) + "ms", t);
- sleepTime = 100;
- i--; // reset the try count
- logRetries = false;
+ } else {
+ // RS is already processing this region, only need to update the timestamp
+ LOG.debug("update " + state + " the timestamp.");
+ state.updateTimestampToNow();
+ if (maxWaitTime < 0) {
+ maxWaitTime = EnvironmentEdgeManager.currentTime() + conf.getLong(
+ ALREADY_IN_TRANSITION_WAITTIME, DEFAULT_ALREADY_IN_TRANSITION_WAITTIME);
+ }
+ long now = EnvironmentEdgeManager.currentTime();
+ if (now < maxWaitTime) {
+ LOG.debug("Region is already in transition; " + "waiting up to "
+ + (maxWaitTime - now) + "ms",
+ t);
+ sleepTime = 100;
+ i--; // reset the try count
+ logRetries = false;
+ }
+ }
}
- }
- }
-
- try {
- if (sleepTime > 0) {
- Thread.sleep(sleepTime);
- }
- } catch (InterruptedException ie) {
- LOG.warn("Failed to unassign "
- + region.getRegionNameAsString() + " since interrupted", ie);
- Thread.currentThread().interrupt();
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
- }
- return;
- }
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
+ // If sleepTime is not set by any of the cases, set it to sleep for
+ // configured exponential backoff time
+ if (sleepTime == 0 && i != maximumAttempts) {
+ sleepTime = backoffPolicy.getBackoffTime(retryConfig, i);
+ LOG.info("Waiting for " + sleepTime + "milliseconds exponential backoff time for "
+ + region.getRegionNameAsString() + " before next retry " + (i + 1) + " of "
+ + this.maximumAttempts);
+ }
+ try {
+ if (sleepTime > 0 && i != maximumAttempts) {
+ Thread.sleep(sleepTime);
+ }
+ } catch (InterruptedException ie) {
+ LOG.warn("Failed to unassign " + region.getRegionNameAsString() + " since interrupted", ie);
+ Thread.currentThread().interrupt();
+ if (state != null) {
Review comment:
do this before propagating the interrupt flag?
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java
##########
@@ -598,6 +601,68 @@ public void testCloseFailed() throws Exception {
}
}
+ /**
+ * This tests region close with exponential backoff
+ */
+ @Test(timeout = 60000)
+ public void testCloseRegionWithExponentialBackOff() throws Exception {
+ String table = "testCloseRegionWithExponentialBackOff";
+ // Set the backoff time between each retry for failed close
+ TEST_UTIL.getMiniHBaseCluster().getConf().setLong("hbase.assignment.retry.sleep.initial", 1000);
+ HMaster activeMaster = TEST_UTIL.getHBaseCluster().getMaster();
+ TEST_UTIL.getMiniHBaseCluster().stopMaster(activeMaster.getServerName());
+ TEST_UTIL.getMiniHBaseCluster().startMaster(); // restart the master for conf take into affect
+
+ try {
+ ScheduledThreadPoolExecutor scheduledThreadPoolExecutor =
+ new ScheduledThreadPoolExecutor(1, Threads.newDaemonThreadFactory("ExponentialBackOff"));
+
+ HTableDescriptor desc = new HTableDescriptor(TableName.valueOf(table));
+ desc.addFamily(new HColumnDescriptor(FAMILY));
+ admin.createTable(desc);
+
+ Table meta = new HTable(conf, TableName.META_TABLE_NAME);
+ HRegionInfo hri =
+ new HRegionInfo(desc.getTableName(), Bytes.toBytes("A"), Bytes.toBytes("Z"));
+ MetaTableAccessor.addRegionToMeta(meta, hri);
+
+ HMaster master = TEST_UTIL.getHBaseCluster().getMaster();
+ AssignmentManager am = master.getAssignmentManager();
+ assertTrue(TEST_UTIL.assignRegion(hri));
+ ServerName sn = am.getRegionStates().getRegionServerOfRegion(hri);
+ TEST_UTIL.assertRegionOnServer(hri, sn, 6000);
+
+ MyRegionObserver.preCloseEnabled.set(true);
+ // Unset the precloseEnabled flag after 1 second for next retry to succeed
+ scheduledThreadPoolExecutor.schedule(new Runnable() {
+ @Override
+ public void run() {
+ MyRegionObserver.preCloseEnabled.set(false);
Review comment:
Does the test reliably fail without the patch, meaning it ends up in FAILED_CLOSE?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r432888949
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerOnCluster.java
##########
@@ -598,6 +605,68 @@ public void testCloseFailed() throws Exception {
}
}
+ /**
+ * This tests region close with exponential backoff
+ */
+ @Test(timeout = 60000)
+ public void testCloseRegionWithExponentialBackOff() throws Exception {
+ String table = testTableName.getTableName().toString();
Review comment:
you already get a "TableName", you don't need to convert it to string and back to TableName. Also, TEST_UTIL.deleteTable() takes TableName as argument.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on a change in pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on a change in pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#discussion_r430780670
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1997,80 +2007,76 @@ private void unassign(final HRegionInfo region,
}
try {
// Send CLOSE RPC
- if (serverManager.sendRegionClose(server, region,
- versionOfClosingNode, dest, transitionInZK)) {
- LOG.debug("Sent CLOSE to " + server + " for region " +
- region.getRegionNameAsString());
+ if (serverManager.sendRegionClose(server, region, versionOfClosingNode, dest,
+ transitionInZK)) {
+ LOG.debug("Sent CLOSE to " + server + " for region " + region.getRegionNameAsString());
if (useZKForAssignment && !transitionInZK && state != null) {
// Retry to make sure the region is
// closed so as to avoid double assignment.
- unassign(region, state, versionOfClosingNode,
- dest, transitionInZK, src);
+ unassign(region, state, versionOfClosingNode, dest, transitionInZK, src);
}
return;
}
// This never happens. Currently regionserver close always return true.
// Todo; this can now happen (0.96) if there is an exception in a coprocessor
- LOG.warn("Server " + server + " region CLOSE RPC returned false for " +
- region.getRegionNameAsString());
+ LOG.warn("Server " + server + " region CLOSE RPC returned false for "
+ + region.getRegionNameAsString());
} catch (Throwable t) {
long sleepTime = 0;
Configuration conf = this.server.getConfiguration();
if (t instanceof RemoteException) {
- t = ((RemoteException)t).unwrapRemoteException();
+ t = ((RemoteException) t).unwrapRemoteException();
}
boolean logRetries = true;
- if (t instanceof RegionServerAbortedException
- || t instanceof RegionServerStoppedException
+ if (t instanceof RegionServerAbortedException || t instanceof RegionServerStoppedException
|| t instanceof ServerNotRunningYetException) {
// RS is aborting or stopping, we cannot offline the region since the region may need
- // to do WAL recovery. Until we see the RS expiration, we should retry.
+ // to do WAL recovery. Until we see the RS expiration, we should retry.
sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
RpcClient.FAILED_SERVER_EXPIRY_DEFAULT);
} else if (t instanceof NotServingRegionException) {
- LOG.debug("Offline " + region.getRegionNameAsString()
- + ", it's not any more on " + server, t);
+ LOG.debug(
+ "Offline " + region.getRegionNameAsString() + ", it's not any more on " + server, t);
if (transitionInZK) {
deleteClosingOrClosedNode(region, server);
}
if (state != null) {
regionOffline(region);
}
return;
- } else if ((t instanceof FailedServerException) || (state != null &&
- t instanceof RegionAlreadyInTransitionException)) {
- if (t instanceof FailedServerException) {
- sleepTime = 1L + conf.getInt(RpcClient.FAILED_SERVER_EXPIRY_KEY,
+ } else if ((t instanceof FailedServerException)
Review comment:
Yes, there is no change in this section
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
Review comment:
The idea is to use the exponential backoff configs "hbase.assignment.retry.sleep.initial" and "hbase.assignment.retry.sleep.initial" for backoff between retries as they can be exhausted pretty fast in case where the server is loaded /busy and cannot really even acknowledge the region close request from the master. We need to use them to schedule the retry at a later point in a different thread asynchronouly
The sleepTime is not really meant for this use case and is not reading any exponential backoff configs
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -1972,6 +1975,13 @@ private void unassign(final HRegionInfo region,
final RegionState state, final int versionOfClosingNode,
final ServerName dest, final boolean transitionInZK,
final ServerName src) {
+ String encodedName = region.getEncodedName();
+ AtomicInteger failedCloseCount = failedCloseTracker.get(encodedName);
+ if (failedCloseCount == null) {
+ failedCloseCount = new AtomicInteger();
+ failedCloseTracker.put(encodedName, failedCloseCount);
Review comment:
I agree, that's what we do even for failedOpenTracker. I can make the change for forceRegionStateToOffline() to take the lock before changing the state
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -2079,16 +2085,29 @@ private void unassign(final HRegionInfo region,
}
if (logRetries) {
- LOG.info("Server " + server + " returned " + t + " for "
- + region.getRegionNameAsString() + ", try=" + i
- + " of " + this.maximumAttempts, t);
+ LOG.info("Server " + server + " returned " + t + " for " + region.getRegionNameAsString()
+ + ", try=" + i + " of " + this.maximumAttempts,
+ t);
// Presume retry or server will expire.
}
}
}
- // Run out of attempts
- if (state != null) {
- regionStates.updateRegionState(region, State.FAILED_CLOSE);
+
+ long sleepTime = backoffPolicy.getBackoffTime(retryConfig,
+ getFailedAttempts(encodedName, failedCloseTracker));
+ if (failedCloseCount.incrementAndGet() <= maximumAttempts && sleepTime > 0) {
+ if (failedCloseTracker.containsKey(encodedName)) {
+ // Sleep before trying unassign if this region has failed to close before
+ scheduledThreadPoolExecutor.schedule(new DelayedUnAssignCallable(this, region, state,
Review comment:
As mentioned, this mainly deals with the case where the RS is even busy to acknowledge the request from Master and tries to have a backoff before next retry to avoid the FAILED_CLOSE state if possible. In the case where we get NotServingRegionException, we still offline the region.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] sguggilam commented on pull request #1755: HBASE-24069 Provide an ExponentialBackOffPolicy sleep between failed …
Posted by GitBox <gi...@apache.org>.
sguggilam commented on pull request #1755:
URL: https://github.com/apache/hbase/pull/1755#issuecomment-632369851
@apurtell for review
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org