Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-22283 Print row and table information when failed to get region location #181

Merged
merged 3 commits into from
Apr 24, 2019

Conversation

carp84
Copy link
Member

@carp84 carp84 commented Apr 22, 2019

What is the purpose of the change

This PR refines the client logging message on RpcRetryingCallerWithReadReplicas.getRegionLocations for easier debugging.

Brief change log

  • Add row and table name into the logging message of RpcRetryingCallerWithReadReplicas.getRegionLocations when failed to get region location or the location got is null.

Verifying this change

This change could be verified by existing tests.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The runtime code path (performance sensitive): (no)
  • Anything that affects deployment or recovery: AssignmentManager, WAL system, ZooKeeper: (no)
  • File system relative (HDFS/S3): (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 276 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.
-0 test4tests 0 The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 mvninstall 323 master passed
+1 compile 30 master passed
+1 checkstyle 40 master passed
+1 shadedjars 337 branch has no errors when building our shaded downstream artifacts.
+1 findbugs 77 master passed
+1 javadoc 27 master passed
_ Patch Compile Tests _
+1 mvninstall 313 the patch passed
+1 compile 31 the patch passed
+1 javac 31 the patch passed
-1 checkstyle 40 hbase-client: The patch generated 1 new + 7 unchanged - 1 fixed = 8 total (was 8)
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 341 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 642 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 87 the patch passed
+1 javadoc 28 the patch passed
_ Other Tests _
+1 unit 229 hbase-client in the patch passed.
+1 asflicense 13 The patch does not generate ASF License warnings.
2900
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-181/1/artifact/out/Dockerfile
GITHUB PR #181
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux f95f81aa8034 4.4.0-145-generic #171-Ubuntu SMP Tue Mar 26 12:43:40 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision master / b0075a1
maven version: Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
Default Java 1.8.0_181
findbugs v3.1.11
checkstyle https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-181/1/artifact/out/diff-checkstyle-hbase-client.txt
Test Results https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-181/1/testReport/
Max. process+thread count 293 (vs. ulimit of 10000)
modules C: hbase-client U: hbase-client
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-181/1/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

@carp84
Copy link
Member Author

carp84 commented Apr 22, 2019

Checking hbase-checkstyle/src/main/resources/hbase/checkstyle.xml we could see the rule is to put all shaded classes at end of the class, which was not the case of RpcRetryingCallerWithReadReplicas, and we will fix it in this PR.

    <module name="ImportOrder">
      <property name="groups" value="*,org.apache.hbase.thirdparty,org.apache.hadoop.hbase.shaded"/>
      <property name="option" value="top" />
      <property name="ordered" value="true"/>
      <property name="sortStaticImportsAlphabetically" value="true"/>
    </module>

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 52 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.
-0 test4tests 0 The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 mvninstall 299 master passed
+1 compile 29 master passed
+1 checkstyle 38 master passed
+1 shadedjars 338 branch has no errors when building our shaded downstream artifacts.
+1 findbugs 76 master passed
+1 javadoc 27 master passed
_ Patch Compile Tests _
+1 mvninstall 310 the patch passed
+1 compile 30 the patch passed
+1 javac 30 the patch passed
+1 checkstyle 39 hbase-client: The patch generated 0 new + 5 unchanged - 3 fixed = 5 total (was 8)
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 347 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 645 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 88 the patch passed
+1 javadoc 28 the patch passed
_ Other Tests _
+1 unit 226 hbase-client in the patch passed.
+1 asflicense 13 The patch does not generate ASF License warnings.
2654
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-181/2/artifact/out/Dockerfile
GITHUB PR #181
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux 7fc026d3d4d4 4.4.0-145-generic #171-Ubuntu SMP Tue Mar 26 12:43:40 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision master / b0075a1
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-181/2/testReport/
Max. process+thread count 295 (vs. ulimit of 10000)
modules C: hbase-client U: hbase-client
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-181/2/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

@carp84
Copy link
Member Author

carp84 commented Apr 22, 2019

pre-commit check looks good, waiting for review, thanks.

Copy link
Contributor

@saintstack saintstack left a comment

Choose a reason for hiding this comment

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

Patch looks good except for part where we are pre-making error string though we only use it when a problem. Do we have to do this? Can we not just make the error string when an error? Thanks @carp84

@carp84
Copy link
Member Author

carp84 commented Apr 23, 2019

Thanks for review @saintstack, have just updated the PR following the suggestion.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
0 reexec 45 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.
-0 test4tests 0 The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 mvninstall 317 master passed
+1 compile 30 master passed
+1 checkstyle 39 master passed
+1 shadedjars 340 branch has no errors when building our shaded downstream artifacts.
+1 findbugs 78 master passed
+1 javadoc 28 master passed
_ Patch Compile Tests _
+1 mvninstall 307 the patch passed
+1 compile 31 the patch passed
+1 javac 31 the patch passed
+1 checkstyle 37 hbase-client: The patch generated 0 new + 5 unchanged - 3 fixed = 5 total (was 8)
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedjars 345 patch has no errors when building our shaded downstream artifacts.
+1 hadoopcheck 625 Patch does not cause any errors with Hadoop 2.7.4 or 3.0.0.
+1 findbugs 89 the patch passed
+1 javadoc 26 the patch passed
_ Other Tests _
+1 unit 236 hbase-client in the patch passed.
+1 asflicense 12 The patch does not generate ASF License warnings.
2647
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-181/3/artifact/out/Dockerfile
GITHUB PR #181
Optional Tests dupname asflicense javac javadoc unit findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
uname Linux 8bbd881fa6f8 4.4.0-145-generic #171-Ubuntu SMP Tue Mar 26 12:43:40 UTC 2019 x86_64 GNU/Linux
Build tool maven
Personality /testptch/patchprocess/precommit/personality/provided.sh
git revision master / 1584d24
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-181/3/testReport/
Max. process+thread count 294 (vs. ulimit of 10000)
modules C: hbase-client U: hbase-client
Console output https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-181/3/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

Copy link
Contributor

@saintstack saintstack left a comment

Choose a reason for hiding this comment

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

Yeah, it dupes code but this is better I think. Thanks @carp84

@carp84 carp84 merged commit ab3d6cf into apache:master Apr 24, 2019
@carp84
Copy link
Member Author

carp84 commented Apr 24, 2019

Thanks @saintstack, merged into master and manually pushed into branch-1/branch-2

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