Skip to content

Conversation

@Apache9
Copy link
Contributor

@Apache9 Apache9 commented Apr 28, 2019

Still have some problems on implementing UTs, will update the patch soon. And maybe we could change the config name? I think a more general reason to use a larger pause is that the region server is overloaded, CallQueueTooBigException is only one possible result of a overloaded region server. So maybe we could change the config and also the new methods in TableBuilder and AdminBuilder to something like pauseWhenOverloaded? What do you think @carp84 . Thanks.

@Apache9 Apache9 requested a review from carp84 April 28, 2019 15:03
@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 51 Docker mode activated.
_ Prechecks _
+1 hbaseanti 0 Patch does not have any anti-patterns.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ master Compile Tests _
0 mvndep 30 Maven dependency ordering for branch
+1 mvninstall 274 master passed
+1 compile 96 master passed
+1 checkstyle 122 master passed
+1 shadedjars 294 branch has no errors when building our shaded downstream artifacts.
+1 findbugs 296 master passed
+1 javadoc 54 master passed
_ Patch Compile Tests _
0 mvndep 15 Maven dependency ordering for patch
+1 mvninstall 248 the patch passed
+1 compile 76 the patch passed
+1 javac 76 the patch passed
+1 checkstyle 32 hbase-client: The patch generated 0 new + 21 unchanged - 9 fixed = 21 total (was 30)
+1 checkstyle 72 The patch passed checkstyle in hbase-server
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 272 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 555 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 403 the patch passed
+1 javadoc 55 the patch passed
_ Other Tests _
+1 unit 190 hbase-client in the patch passed.
+1 unit 12299 hbase-server in the patch passed.
+1 asflicense 47 The patch does not generate ASF License warnings.
15580
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/1/artifact/out/Dockerfile
GITHUB PR #195
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux 1a45d0f6bf0b 4.4.0-141-generic #167~14.04.1-Ubuntu SMP Mon Dec 10 13:20:24 UTC 2018 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision master / 4477dd5
maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
Default Java 1.8.0_181
findbugs v3.1.11
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/1/testReport/
Max. process+thread count 4735 (vs. ulimit of 10000)
modules C: hbase-client hbase-server U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/1/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

@carp84
Copy link
Member

carp84 commented Apr 29, 2019

Maybe we could change the config name? I think a more general reason to use a larger pause is that the region server is overloaded, CallQueueTooBigException is only one possible result of an overloaded region server. So maybe we could change the config and also the new methods in TableBuilder and AdminBuilder to something like pauseWhenOverloaded?

Agreed. Actually the original config name proposed in HBASE-17114 is hbase.client.pause.special but was concerned in this comment thus changed to some more specific one to cqtbe.

Will be back and review the PR soon.

* times) are both limitations for retrying, we will stop retrying when we reach any of the
* limitations.
* @param timeout
* @param unit
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better to add some javadoc to parameters here instead of removing? I mean after all we are changing this part, let's improve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not have javadoc for these parameters in both AsyncTableBuilder and TableBuilder, and I think the parameter name is good enough to tell users the meaning?

Copy link
Member

Choose a reason for hiding this comment

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

It will generate checkstyle error by default although not a big deal.

/**
* Set timeout for each rpc request.
* @param timeout
* @param unit
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

* Set the base pause time for retrying. We use an exponential policy to generate sleep time when
* retrying.
* @param timeout
* @param unit
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

* Set the max retry times for an admin operation. Usually it is the max attempt times minus 1.
* Operation timeout and max attempt times(or max retry times) are both limitations for retrying,
* we will stop retrying when we reach any of the limitations.
* @param maxRetries
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

* Set the max attempt times for an admin operation. Usually it is the max retry times plus 1.
* Operation timeout and max attempt times(or max retry times) are both limitations for retrying,
* we will stop retrying when we reach any of the limitations.
* @param maxAttempts
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.


/**
* Set the number of retries that are allowed before we start to log.
* @param startLogErrorsCnt
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member

@carp84 carp84 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just some minor comments that don't block the commit. And please make it compatible for old configuration if plan to change the config name.

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 295 Docker mode activated.
_ Prechecks _
+1 hbaseanti 0 Patch does not have any anti-patterns.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ master Compile Tests _
0 mvndep 26 Maven dependency ordering for branch
+1 mvninstall 296 master passed
+1 compile 99 master passed
+1 checkstyle 125 master passed
+1 shadedjars 332 branch has no errors when building our shaded downstream artifacts.
+1 findbugs 364 master passed
+1 javadoc 67 master passed
_ Patch Compile Tests _
0 mvndep 17 Maven dependency ordering for patch
+1 mvninstall 294 the patch passed
+1 compile 98 the patch passed
+1 javac 98 the patch passed
+1 checkstyle 38 hbase-client: The patch generated 0 new + 21 unchanged - 9 fixed = 21 total (was 30)
+1 checkstyle 77 The patch passed checkstyle in hbase-server
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 329 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 619 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 382 the patch passed
+1 javadoc 66 the patch passed
_ Other Tests _
+1 unit 222 hbase-client in the patch passed.
-1 unit 15840 hbase-server in the patch failed.
+1 asflicense 56 The patch does not generate ASF License warnings.
19747
Reason Tests
Failed junit tests hadoop.hbase.client.TestAsyncTableAdminApi
hadoop.hbase.client.TestSnapshotTemporaryDirectoryWithRegionReplicas
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/2/artifact/out/Dockerfile
GITHUB PR #195
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux 6341fd104ff4 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision master / 4477dd5
maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
Default Java 1.8.0_181
findbugs v3.1.11
unit https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/2/artifact/out/patch-unit-hbase-server.txt
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/2/testReport/
Max. process+thread count 5123 (vs. ulimit of 10000)
modules C: hbase-client hbase-server U: .
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-195/2/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

@Apache9
Copy link
Contributor Author

Apache9 commented Apr 29, 2019

Agreed. Actually the original config name proposed in HBASE-17114 is hbase.client.pause.special but was concerned in this comment thus changed to some more specific one to cqtbe.

OK, can do this in another issue...

@Apache9 Apache9 merged commit f9f6354 into apache:master Apr 30, 2019
Sign up for free to 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.

3 participants