Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10447 )
Change subject: IMPALA-7061: Rework HBase splitting and assignment ...................................................................... Patch Set 6: (5 comments) Thanks for tackling this. This looks great. I didn't manually look at the diff of HBaseTestDataRegionAssignment, but I assume you just removed the splitting stuff. If you did more interesting surgery, let me know, and I'll take a more careful look. I think we can move the splitpoints into the template.sql file, but I'm ok either way. http://gerrit.cloudera.org:8080/#/c/10447/6/fe/src/compat-minicluster-profile-2/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java File fe/src/compat-minicluster-profile-2/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java: http://gerrit.cloudera.org:8080/#/c/10447/6/fe/src/compat-minicluster-profile-2/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java@48 PS6, Line 48: * Splits HBase tables into regions and deterministically assigns regions to region Gerrit is lame about not detecting the rename here and showing a useful diff. It looks like you removed the splitting part of the code. if so, update the comments? http://gerrit.cloudera.org:8080/#/c/10447/6/fe/src/compat-minicluster-profile-2/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java@142 PS6, Line 142: public static String printKey(byte[] key) { Remove this and use fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java's version? http://gerrit.cloudera.org:8080/#/c/10447/6/fe/src/compat-minicluster-profile-3/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java File fe/src/compat-minicluster-profile-3/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java: http://gerrit.cloudera.org:8080/#/c/10447/6/fe/src/compat-minicluster-profile-3/test/java/org/apache/impala/datagenerator/HBaseTestDataRegionAssignment.java@168 PS6, Line 168: public static String printKey(byte[] key) { same here; we have another copy of this. http://gerrit.cloudera.org:8080/#/c/10447/6/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: http://gerrit.cloudera.org:8080/#/c/10447/6/testdata/datasets/functional/functional_schema_template.sql@144 PS6, Line 144: True Instead of this being a boolean, do you want to put the splits values here? It means that there will be two copies (for the two relevant table), but that seems correct. http://gerrit.cloudera.org:8080/#/c/10447/6/testdata/datasets/functional/functional_schema_template.sql@521 PS6, Line 521: True same -- To view, visit http://gerrit.cloudera.org:8080/10447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d639128a856254a6ccb93d6750f531974b5f897 Gerrit-Change-Number: 10447 Gerrit-PatchSet: 6 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Wed, 23 May 2018 19:33:02 +0000 Gerrit-HasComments: Yes
