Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9716 )
Change subject: IMPALA-4277: Support multiple versions of Hadoop ecosystem ...................................................................... Patch Set 2: Code-Review+1 (5 comments) I did a pass over it. I think most of this code has already has eyes on it so it all looks pretty reasonable (considering that it is a bunch of slightly gross hacks by necessity). http://gerrit.cloudera.org:8080/#/c/9716/2/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/SentryAuthProvider.java File fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/SentryAuthProvider.java: http://gerrit.cloudera.org:8080/#/c/9716/2/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/SentryAuthProvider.java@73 PS2, Line 73: new Object[] {policyFile, engine, HivePrivilegeModel.getInstance()}); nit: extra space http://gerrit.cloudera.org:8080/#/c/9716/2/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java File fe/src/main/java/org/apache/impala/util/SentryPolicyService.java: http://gerrit.cloudera.org:8080/#/c/9716/2/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java@398 PS2, Line 398: return Lists.newArrayList(SentryUtil.listRoles(client.get(), requestingUser.getShortName())); nit: long line http://gerrit.cloudera.org:8080/#/c/9716/2/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test File testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test: http://gerrit.cloudera.org:8080/#/c/9716/2/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test@368 PS2, Line 368: set parquet_read_statistics=0; This isn't needed, right, since it's already set in Python. http://gerrit.cloudera.org:8080/#/c/9716/2/tests/query_test/test_mt_dop.py File tests/query_test/test_mt_dop.py: http://gerrit.cloudera.org:8080/#/c/9716/2/tests/query_test/test_mt_dop.py@103 PS2, Line 103: vector.get_value('exec_option')['parquet_read_statistics'] = '0' Comment here to explain why, e.g. "Disable min-max stats to prevent interference with dictionary filtering." http://gerrit.cloudera.org:8080/#/c/9716/2/tests/query_test/test_partitioning.py File tests/query_test/test_partitioning.py: http://gerrit.cloudera.org:8080/#/c/9716/2/tests/query_test/test_partitioning.py@68 PS2, Line 68: is_hive_2 = os.environ.get("IMPALA_MINICLUSTER_PROFILE", None) == "3" Can we move this to a helper function in tests/common/environ.py. It's somewhat overkill when there are only two occurrences but it would be good to avoid this pattern spreading any further. -- To view, visit http://gerrit.cloudera.org:8080/9716 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a2ab50331986c7394c2bbfd6c865232bca975f7 Gerrit-Change-Number: 9716 Gerrit-PatchSet: 2 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 20 Mar 2018 19:47:57 +0000 Gerrit-HasComments: Yes
