[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 4 files changed, 129 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/8 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 8 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12977 ) Change subject: IMPALA-5351: Support storing column comment of kudu table .. Patch Set 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2719/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 10 Apr 2019 02:41:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12977 to look at the new patch set (#2). Change subject: IMPALA-5351: Support storing column comment of kudu table .. IMPALA-5351: Support storing column comment of kudu table This patch intends to support storing column comment of kudu table on impala side. Belows tests passed: 1) creata kudu table with column comment; 2) alter kudu table with (add/alter[delete] column comment); 3) show create kudu table; 4) describe kudu table; Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c --- M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java M fe/src/main/java/org/apache/impala/catalog/KuduColumn.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java 3 files changed, 21 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/12977/2 -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: helifu
[Impala-ASF-CR] IMPALA-5843: Use page index in Parquet files to skip pages
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12065 ) Change subject: IMPALA-5843: Use page index in Parquet files to skip pages .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/12065/9/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/12065/9/common/thrift/ImpalaService.thrift@387 PS9, Line 387: READ_PARQUET_PAGE_INDEX = 79 > All other Parquet query options start with PARQUET_..., can we do the same On second thought, why don't we guard this whole effort behind PARQUET_READ_STATISTICS. We already have too many query options and it might not be worth to add another one when we can just guard it all by one. -- To view, visit http://gerrit.cloudera.org:8080/12065 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0cc99f129f2048dbafbe7f5a51d1ea3a5005731a Gerrit-Change-Number: 12065 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 10 Apr 2019 01:47:34 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2718/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Krishna Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Wed, 10 Apr 2019 01:47:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12977 ) Change subject: IMPALA-5351: Support storing column comment of kudu table .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2717/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 1 Gerrit-Owner: helifu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 10 Apr 2019 01:26:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 7:
(14 comments)
Thanks for the review Vihang. Updated patch with comments resolved.
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@8
PS4, Line 8: incorrect deletion of Database
> I think this is not accurate. creation_time is not used for invalidating bu
Done
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@15
PS4, Line 15: we can skip the event
: instead.
> suggest you to change it to ".. and we can skip the event instead" which is
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h
File be/src/util/event-metrics.h:
http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h@40
PS4, Line 40: static DoubleGauge* EVENTS_FETCH_DURATION_MEAN;
> Is there any specific value in exposing this metric on the Catalog metrics
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@953
PS4, Line 953: public List getTableProperties(
> like suggested in my other comment related to race conditions , this API in
Removed this method
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@208
PS4, Line 208:
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC)
> Just having this line is sufficient to expose the metric in /events page. Y
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@898
PS4, Line 898: vent's DB object, it means that the Database object present
> The process method does not invalidate so this description needs to be upda
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@899
PS4, Line 899: * in the catalog is a later version and we can skip the
event.
> line too long (100 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@903
PS4, Line 903: verride
: public void process() {
: Reference dbFound = new Reference<>();
: Reference dbMatched = new Reference<>();
: Db removedDb =
catalog_.removeDbIfExists(droppedDatabase_, dbFound, dbMatched);
: if (removedDb != null) {
: infoLog("Removed Database {} ", dbName_);
: } else if (!dbMatched.getRef()) {
: LOG.warn(debugString("Database %s was not removed
from catalog since "
: + "the creation time of the Database did not
match", dbName_));
:
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC).inc();
: } else if (!dbFound.getRef()) {
: debugLog("Database {} was not removed since it did
not exist in catalog
> This code has potential race conditions. For example, the createTime of the
Refactored this method similar to DropTable. Now atomically checking
CREATE_TIME and removing DB
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@272
PS4, Line 272:* DROP_DATABASE uses CREATION_TIME to filter events that try
to drop an
> add javadoc to describe what exactly this test is testing. Specifically, de
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@284
PS4, Line 284:
> add javadoc
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@285
PS4, Line 285: // Check that the database exists in Catalog
> line too long (126 > 90)
Done
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@293
PS4, Line 293:*/
> line too long (91 > 90)
Done
http://gerrit.cloudera.
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid
incorrect deletion of Database objects.
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/7/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@915
PS7, Line 915: debugLog("Database {} was not removed since it did
not exist in catalog.", dbName_);
line too long (96 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12938
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c
Gerrit-Change-Number: 12938
Gerrit-PatchSet: 7
Gerrit-Owner: Bharath Krishna
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Krishna
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Wed, 10 Apr 2019 01:02:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12977 ) Change subject: IMPALA-5351: Support storing column comment of kudu table .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12977/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12977/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@302 PS1, Line 302: cols.add(new FieldSchema(colSchema.getName(), type.toSql().toLowerCase(), comment)); > line too long (92 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 1 Gerrit-Owner: helifu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 10 Apr 2019 01:02:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME in event processor to avoid incorrect deletion of Database objects.
Bharath Krishna has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12938 ) Change subject: IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. .. IMPALA-8338 : Check CREATION_TIME in event processor to avoid incorrect deletion of Database objects. Process the drop database event only if the CREATION_TIME of the catalog's database object is lesser than or equal to that of the database object present in the notification event. If the CREATION_TIME in the notification event object is lesser than the catalog's DB object, it means that the Database object present in the catalog is the latest and we can skip the event instead. Testing : - Added unit tests in MetastoreEventsProcessorTest. - Enabled testCreateDropCreateDatabaseFromImpala as we now have CREATION_TIME in the notification events. Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java 4 files changed, 128 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/12938/7 -- To view, visit http://gerrit.cloudera.org:8080/12938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8fd3685cf7261e4953f4f884850489a47c5bbd6c Gerrit-Change-Number: 12938 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12977 ) Change subject: IMPALA-5351: Support storing column comment of kudu table .. Patch Set 1: The latest kudu client is required, otherwise the compilation fails -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 1 Gerrit-Owner: helifu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: helifu Gerrit-Comment-Date: Wed, 10 Apr 2019 00:47:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12977 ) Change subject: IMPALA-5351: Support storing column comment of kudu table .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12977/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12977/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@302 PS1, Line 302: cols.add(new FieldSchema(colSchema.getName(), type.toSql().toLowerCase(), comment)); line too long (92 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 1 Gerrit-Owner: helifu Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Apr 2019 00:43:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 7 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 10 Apr 2019 00:34:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5351: Support storing column comment of kudu table
helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12977 Change subject: IMPALA-5351: Support storing column comment of kudu table .. IMPALA-5351: Support storing column comment of kudu table This patch intends to support storing column comment of kudu table on impala side. Belows tests passed: 1) creata kudu table with column comment; 2) alter kudu table with (add/alter[delete] column comment); 3) show create kudu table; 4) describe kudu table; Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c --- M fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java M fe/src/main/java/org/apache/impala/catalog/KuduColumn.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java 3 files changed, 19 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/12977/1 -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 1 Gerrit-Owner: helifu
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. IMPALA-8227: Add support for WITH GRANT OPTION with Ranger This patch adds support for the WITH GRANT OPTION SQL syntax when granting privileges to users and groups in Ranger. This allows users who have been granted a privilege to then grant that privilege to other users/groups. Testing: - Ran all FE tests - Ran authorization E2E tests - Added an E2E authorization test in test_ranger to verify the functionality Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Reviewed-on: http://gerrit.cloudera.org:8080/12962 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M tests/authorization/test_ranger.py 2 files changed, 97 insertions(+), 6 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 8 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2716/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Apr 2019 00:34:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7971: Add support for insert events in event processor.
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12889 )
Change subject: IMPALA-7971: Add support for insert events in event processor.
..
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/12889/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/12889/6/be/src/service/client-request-state.cc@1106
PS6, Line 1106: // is_overwrite is used to know the type of insert in FE.
nit: I think this is kind of obv (and also there is a comment in the thrift
def). Consider removing.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2063
PS6, Line 2063: Db/table doesn't exist.
Looks like it throws DbNotFoundEx if the db doesn't exist.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2065
PS6, Line 2065: * - Throws CatalogException if reloadTable() is unsuccessful.
:* - Returns true only if reloadTable() succeeds.
nit: Just say, returns true if reload is successful, false otherwise? Remove
hyphens if not needed.
(same below)
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2077
PS6, Line 2077: Useful to refresh the partition after inserts
I feel the entire statement can be omitted. Callers may change that makes the
doc stale.?
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2082
PS6, Line 2082:* - Returns true only if reloadPartition() succeeds.
same comments as above javadoc.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@835
PS6, Line 835:
nit: Returns..
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@398
PS6, Line 398:protected String getPartitionKeyValuesAsString(Map partSpec) {
: StringBuilder str = new StringBuilder();
: for (Map.Entry entry :
partSpec.entrySet()) {
: str.append(entry.getKey() + "=" + entry.getValue() + "
");
: }
: return str.toString();
: }
Use Joiner from guava
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@630
PS6, Line 630: * Currently we do not check for self-events in Inserts.
This means insert events
Any reason why?
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@635
PS6, Line 635: public void process() throws MetastoreNotificationException {
can we break this into two helpers? processPartitionedInsert() and
processNonPartitionedInsert()? (for readability, since there are two many
nested branches.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3558
PS6, Line 3558: HiveConf hiveConf = new HiveConf(this.getClass());
any reason to move this to here from where it was before
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3578
PS6, Line 3578:
filesBeforeInsertForPartitions.put(partition.getId(),
> Can we do this conditionally when event processing is enabled?
+1, this is a very commonly used codepth and seems wasteful to unpack the file
descriptor info if event processing is not configured.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3674
PS6, Line 3674: Preconditions.checkState(parts.size() == 1);
This is taken care of by Iterabls.getOnlyElement()
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3721
PS6, Line 3721:*/
document method args? Map doesn't seem obvious.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3729
PS6, Line 3729: if (table.getNumClust
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Hello Michael Ho, Lars Volker, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12968 to look at the new patch set (#2). Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb When HdfsScanNode is cancelled or hits and error, SetDoneInternal() holds HdfsScanNode::lock_ while it runs RequestContext::Cancel(), which can wait on IO threads to complete outstanding IOs. This can cause a cascade of blocked threads that causes Prepare() to take a significant time and cause datastream sender timeouts. The specific scenario seen has this set of threads: Thread 1: A DiskIoMgr thread is blocked on IO in hdfsOpenFile() or hdfsRead(), holding HdfsFileReader::lock_. Thread 2: An HDFS scanner thread is blocked in HdfsScanNode::SetDoneInternal() -> RequestContext::Cancel() -> ScanRange::CancelInternal(), waiting on HdfsFileReader::lock_. It is holding HdfsScanNode::lock_. Thread 3: A thread in ThreadResourceMgr::DestroyPool() -> (a few layers) -> HdfsScanNode::ThreadTokenAvailableCb() is blocked waiting on HdfsScanNode::lock_ while holding ThreadResourceMgr::lock_. Thread 4: A thread in FragmentInstanceState::Prepare() -> RuntimeState::Init() -> ThreadResourceMgr::CreatePool() is blocked waiting on ThreadResourceMgr::lock_. When Prepare() takes a significant time, datastream senders will time out waiting for the datastream receivers to start up. This causes failed queries. S3 has higher latencies for IO and does not have file handle caching, so S3 is more susceptible to this issue than other platforms. This changes HdfsScanNode::ThreadTokenAvailableCb() to periodically do a dirty check of HdfsScanNode::done_ when waiting to acquire the lock. This avoids the blocking experienced by Thread 3 in the example above. Testing: - Ran tests on normal HDFS and repeatedly on S3 Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 --- M be/src/common/names.h M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scan-node.h M be/src/runtime/io/request-ranges.h 4 files changed, 45 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/12968/2 -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.h@128 PS1, Line 128: bool done_ = false; > Can we switch this to an atomic? It'll be easier to reason about the safene That makes sense. I switched this to an AtomicBool. http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc@561 PS1, Line 561: // TODO: Cancelling the RequestContext will wait on in-flight IO. We should > Ah I see what you mean and why it's misleading. I updated that comment in request-ranges.h. Let me know if there is a way it could be clearer. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 23:52:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12975 ) Change subject: IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE .. Patch Set 1: Verified+1 Build Successful https://jenkins.impala.io/job/gerrit-docs-auto-test/300/ : Doc tests passed. -- To view, visit http://gerrit.cloudera.org:8080/12975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I704a0623a05166e5f636b6c1063e9feffb58e65b Gerrit-Change-Number: 12975 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sahil Takiar Gerrit-Comment-Date: Tue, 09 Apr 2019 23:52:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12975 ) Change subject: IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE .. Patch Set 1: Build Started https://jenkins.impala.io/job/gerrit-docs-auto-test/300/ Testing docs change - this change appears to modify docs/ and no code. This is experimental - please report any issues to tarmstr...@cloudera.com or on this JIRA: IMPALA-7317 -- To view, visit http://gerrit.cloudera.org:8080/12975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I704a0623a05166e5f636b6c1063e9feffb58e65b Gerrit-Change-Number: 12975 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 23:30:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8401: SIGQUIT initiates the graceful shutdown process
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12973 ) Change subject: IMPALA-8401: SIGQUIT initiates the graceful shutdown process .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2715/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I521ffd7526ac9a8a5c4996994eb68d6a855aef86 Gerrit-Change-Number: 12973 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 23:27:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE
Alex Rodoni has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12975 Change subject: IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE .. IMPALA-8391: [DOCS] ALTER TABLE RENAME now renames the underlying Kudu TABLE Change-Id: I704a0623a05166e5f636b6c1063e9feffb58e65b --- M docs/topics/impala_tables.xml 1 file changed, 33 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/75/12975/1 -- To view, visit http://gerrit.cloudera.org:8080/12975 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I704a0623a05166e5f636b6c1063e9feffb58e65b Gerrit-Change-Number: 12975 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Rodoni
[Impala-ASF-CR] IMPALA-8401: SIGQUIT initiates the graceful shutdown process
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12973 ) Change subject: IMPALA-8401: SIGQUIT initiates the graceful shutdown process .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@62 PS1, Line 62: boost::shared_ptr impala_server; What are the implications of moving this here on the lifetime of impala_server? Before this change it got destructed at the end of ImpaladMain, now it will happen during process shutdown. If we want to keep it, we can go with the previous approach and store an additional pointer here that we set back to nullptr before exiting ImpaladMain below. http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@69 PS1, Line 69: Status status = impala_server->StartShutdown(ONE_YEAR_IN_SECONDS, &shutdown_status); StartShutdown creates a new thread, which might not be re-entrant safe. If we're in the process of handling a client-issued shutdown, then catch this signal, we will possibly interrupt its execution (e.g. the thread creation) and find its global structures in an inconsistent state. We probably should limit this handler to setting a flag, and then initiate the shutdown itself from a separate thread outside the handler. See IMPALA-4796 for a similar issue and kudu/util/minidump.h as an example. http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@71 PS1, Line 71: LOG(ERROR) << "Quit signal received but unable to initiate shutdown. Status: " I think these LOG macros end up not being async-signal safe. In minidump.cc we use a wrapper around the write syscall to log during signal handlers. http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@116 PS1, Line 116: sigaction You should probably memset this to 0 before using it. See minidump.cc for an example. -- To view, visit http://gerrit.cloudera.org:8080/12973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I521ffd7526ac9a8a5c4996994eb68d6a855aef86 Gerrit-Change-Number: 12973 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 23:19:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 ) Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2714/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 23:05:59 + Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Refactor file descriptor loading code
Anonymous Coward (486) has posted comments on this change. ( http://gerrit.cloudera.org:8080/12950 ) Change subject: WIP: Refactor file descriptor loading code .. Patch Set 2: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/12950/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1083 PS2, Line 1083:* objects grouped by their base directory path. Nit: Update javadoc -- To view, visit http://gerrit.cloudera.org:8080/12950 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59edf493b9ba38be5f556b4795a7684d9c9e3a07 Gerrit-Change-Number: 12950 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Anonymous Coward (486) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Reviewer: Yongzhi Chen Gerrit-Comment-Date: Tue, 09 Apr 2019 23:00:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7031: Cancel action of debug page should not unregister query
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12926 ) Change subject: IMPALA-7031: Cancel action of debug page should not unregister query .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2713/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56983d40e0542bc734ec5a66c339b5131b7b56c8 Gerrit-Change-Number: 12926 Gerrit-PatchSet: 5 Gerrit-Owner: Alice Fan Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 22:50:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8401: SIGQUIT initiates the graceful shutdown process
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12973 Change subject: IMPALA-8401: SIGQUIT initiates the graceful shutdown process .. IMPALA-8401: SIGQUIT initiates the graceful shutdown process This patch enables a user that has access to the impalad process, to initiate the graceful shutdown process with a deadline of one year by sending SIGQUIT signal to it. Testing: Added a relevant e2e test. Change-Id: I521ffd7526ac9a8a5c4996994eb68d6a855aef86 --- M be/src/service/impalad-main.cc M tests/custom_cluster/test_restart_services.py 2 files changed, 53 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/12973/1 -- To view, visit http://gerrit.cloudera.org:8080/12973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I521ffd7526ac9a8a5c4996994eb68d6a855aef86 Gerrit-Change-Number: 12973 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc@561 PS1, Line 561: // TODO: Cancelling the RequestContext will wait on in-flight IO. We should > RequestContext::Cancel() calls ScanRange::CancelInternal() - I might miss i Ah I see what you mean and why it's misleading. So when I wrote that comment I was trying to say that "in flight reads have finished" is *not* a post-condition of CancelInternal(). But I can see that that comment could be taken to imply some kind of non-blocking behaviour, and it *can* get blocked on an in-flight I/O because of file_reader_->lock(). So yeah, it could be clarified for sure. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 22:35:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. IMPALA-8385: Refactor Sentry admin user check This patch moves the hardcoded Sentry admin user check from the generic "show roles" and "show grant" catalog operations to Sentry authorization plugin, i.e. SentryImpaladAuthorizationManager. This patch also removes isAdmin() from AuthorizationManager interface. Testing: - Added a new authorization E2E test to test for Sentry admin check - Ran all FE tests - Ran all authorization E2E tests Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Reviewed-on: http://gerrit.cloudera.org:8080/12963 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/catalog/catalog-server.cc M be/src/catalog/catalog.cc M be/src/catalog/catalog.h M be/src/exec/catalog-op-executor.cc M be/src/exec/catalog-op-executor.h M be/src/service/client-request-state.cc M be/src/service/fe-support.cc M common/thrift/CatalogService.thrift M fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A tests/authorization/test_sentry.py 17 files changed, 194 insertions(+), 82 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 22:29:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 ) Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. Patch Set 6: (3 comments) Michael is the obvious person to +2 since he did a bunch of work in codegen and the expr framework. Not sure if he has time to look. http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@41 PS6, Line 41: IMPALA-7331 (CHAR codegen support functions) is also fixed becaus > I am not sure - I do not know how many people use CHAR(N). My intuition say IMPALA-8402 I have seen people use it in production, often as a result from porting over existing schemas from other DBs or using patterns from other DBs. The fixed-width layout does have advantages for short strings (although that's nullified by missing codegen), but the semantics of automatically adding and removing spaces on the end are a little weird and you can't separate the semantics and the memory layout. http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483 PS6, Line 483: // TODO: macroify this? > I would prefer to use a macro in the header too - in such a way that gets t I like this idea. Went ahead with it as you suggested. It was enough to turn this into a net-negative patch in line count (although that's not overly meaningful) http://gerrit.cloudera.org:8080/#/c/12797/7/be/src/udf/udf-internal.h File be/src/udf/udf-internal.h: http://gerrit.cloudera.org:8080/#/c/12797/7/be/src/udf/udf-internal.h@299 PS7, Line 299: // Matches memory layout of StringVal to allow sharing of support in CodegenAnyval. : // Also is denser this can fit into 16 bytes along with the initial is_null member. > This could be enforced with static_assert()s: This is a great idea. Wish I'd thought of it. I assume clang supports it. I will lazily test this assumption with the precommit clang-tidy job :). I had to disable a warning for GCC since it is a stickler about disallowing offsetof for any class with a constructor. -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 22:24:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12797 to look at the new patch set (#8). Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. IMPALA-4356,IMPALA-7331: codegen all ScalarExprs Based on initial draft patch by Pooja Nilangekar. Codegen'd expressions can be executed in two ways - either by being called directly from a fully codegend function, or from interpreted code via a function pointer (previously ScalarFnCall::scalar_fn_wrapper_). This change moves the function pointer from ScalarFnCall to its base class ScalarExpr, so the full expr tree can be codegen'd, not just the ScalarFnCall subtrees. The key refactoring and improvements are: * ScalarExpr::Get*Val() switches between interpreted and the codegen'd function pointer code paths in an inline function, avoiding a virtual function call to ScalarFnCal::Get*Val(). * Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(), which calls a virtual function GetCodegenComputeFnImpl(). * ScalarFnCall's logic for deciding whether to interpret or codegen is better abstracted and exposed to ScalarExpr as IsInterpretable() and ShouldCodegen() methods. * The ScalarExpr::codegend_compute_fn_ function pointer is only populated for expressions that are "codegen entry points". These include the roots of expr trees and non-root expressions where the parent expression calls Get*Val() from the pseudo-codegend GetCodegendComputeFnWrapper(). * ScalarFnCall is always initialised for interpreted execution. Otherwise the function pointer is needed for non-root expressions, e.g. to support ScalarExprEvaluator::GetConstantVal(). * Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal is modified to use the StringVal memory layout to allow code sharing with StringVal. These fixes allowed simplification of IsNotEmptyPredicate codegen (from IMPALA-7657). IMPALA-7331 (CHAR codegen support functions) is also fixed because it was simpler to enable CHAR codegen within ScalarExpr than to carry forward the exiting CHAR workarounds from ScalarFnCall. The CHAR-specific codegen support required in the scalar expr subsystem is very limited. StringVal intermediates are used everywhere. Only SlotRef actually operates on the different tuple layout, and the required codegen support for SlotRef already exists for UDA intermediates anyway. Testing: * Ran exhaustive tests. Perf: * Ran a basic insert benchmark, which went from 10.1s to 7.6s create table foo stored as parquet as select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end from tpch30_parquet.lineitem; * Ran a basic CHAR expr test: set num_nodes=1; set mt_dop=1; select count(*) from lineitem where cast(l_linestatus as CHAR(2)) = 'O ' and cast(l_returnflag as CHAR(2)) = 'N ' The time spent in the scan went from 520ms to 220ms. * Added perf regression test to tpcds-insert, similar to the manual benchmark. * Ran single-node TPC-H with large and small scale factors, to estimate impact on execution perf and query startup time, respectively. +--+---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +--+---+-++++ | TPCH(30) | parquet / none / none | 6.84| -0.18% | 4.49 | -0.31% | +--+---+-++++ +--+--+---++-++---++---++-++ | Workload | Query| File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval | +--+--+---++-++---++---++-++ | TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47| +4.18% | 1.29% | 0.88%| 5 | +4.12% | 2.31| 5.81 | | TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61| +4.33% | 2.18% | 2.15%| 5 | +3.91% | 1.73| 3.09 | | TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50%| 5 | +1.36% | 2.02| 3.94 | | TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65%| 5 | +0.88% | 0.29| 0.73 | | TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35| +1.12% | 1.64% | 1.11%| 5 | +0.80% | 1.15| 1.26 | | TPCH(30) | TPCH-Q14 | parquet / none
[Impala-ASF-CR] IMPALA-7031: Cancel action of debug page should not unregister query
Hello Bikramjeet Vig, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12926 to look at the new patch set (#5). Change subject: IMPALA-7031: Cancel action of debug page should not unregister query .. IMPALA-7031: Cancel action of debug page should not unregister query When a running query is canceled from the Cancel button of queries debug page (impalad WebUI), the query will be unregistered. This can result in a scenario where a client that has a reference to the query handle is unaware that the query has been unregistered via the debug page, and attempts to run an operation on that query handle but encounters and error instead. This patch updates the cancel http endpoint to only cancel the query. Testing: - Added a test to test_web_pages.py - Ran all webserver tests Change-Id: I56983d40e0542bc734ec5a66c339b5131b7b56c8 --- M be/src/service/impala-http-handler.cc M tests/webserver/test_web_pages.py 2 files changed, 22 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/12926/5 -- To view, visit http://gerrit.cloudera.org:8080/12926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I56983d40e0542bc734ec5a66c339b5131b7b56c8 Gerrit-Change-Number: 12926 Gerrit-PatchSet: 5 Gerrit-Owner: Alice Fan Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12282 ) Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2712/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 21:45:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc@561 PS1, Line 561: // TODO: Cancelling the RequestContext will wait on in-flight IO. We should > I think the comment is actually correct - the waiting happens in ScanRange: RequestContext::Cancel() calls ScanRange::CancelInternal() - I might miss it, but I don't see where we call ScanRange::Cancel(), too. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 21:59:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5843: Use page index in Parquet files to skip pages
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12065 ) Change subject: IMPALA-5843: Use page index in Parquet files to skip pages .. Patch Set 9: > (54 comments) > > In a few comments the row group level profile counters came up. > Whether we should only have "NumStatsFilteredRowGroups" and count > row groups that are filtered out by either row group-level stats or > page-level stats. > Or, we should have multiple variants of that counter, counting the > row group-level and page-level filtering separately. What do you > think? For debugging it's good to have more fine-grained data, but > I'm not sure that the query profile is the right place for such > information. I think having counter for row groups filtered based on row group stats, and pages filtered based on page stats would be good (and sufficient). I think entire row groups filtered by page stats (but not row group stats) are not worth tracking in a separate counter. The distinction I'd make is whether we discarded a row group without looking at the index or not. -- To view, visit http://gerrit.cloudera.org:8080/12065 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0cc99f129f2048dbafbe7f5a51d1ea3a5005731a Gerrit-Change-Number: 12065 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 09 Apr 2019 21:55:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5843: Use page index in Parquet files to skip pages
Lars Volker has posted comments on this change. (
http://gerrit.cloudera.org:8080/12065 )
Change subject: IMPALA-5843: Use page index in Parquet files to skip pages
..
Patch Set 9:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/hdfs-parquet-scanner.h
File be/src/exec/parquet/hdfs-parquet-scanner.h:
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/hdfs-parquet-scanner.h@552
PS8, Line 552:
> Added comments. Instead of 'fn_name', I modified this function to take a 's
Left some comment in the call-site: I think pulling that part out of the
function made the calling code slightly less readable.
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/hdfs-parquet-scanner.cc@644
PS8, Line 644:
COUNTER_ADD(num_stats_filtered_row_groups_by_page_index_counter_, 1);
> See reply comment.
Independent of the discussion which counters to maintain, I took a while to
understand how this can happen :). Maybe you want to leave a comment that
explains how the row group statistics will not filter out a column but the
pages will (e.g. with gaps between the pages)? I don't feel strongly about it,
if you think it's obvious, feel free to leave it out.
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/hdfs-parquet-scanner.cc
File be/src/exec/parquet/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/hdfs-parquet-scanner.cc@700
PS9, Line 700: if (stats_field == ColumnStatsReader::StatsField::MIN) {
Use a case-statement, now that it's an enum?
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/hdfs-parquet-scanner.cc@758
PS9, Line 758: const string& fn_name = eval->root().function_name();
: ColumnStatsReader::StatsField stats_field;
: if (fn_name == "lt" || fn_name == "le") {
: stats_field = ColumnStatsReader::StatsField::MIN;
: } else if (fn_name == "gt" || fn_name == "ge") {
: stats_field = ColumnStatsReader::StatsField::MAX;
: } else {
: DCHECK(false) << "Unsupported function name for statistics
evaluation: "
: << fn_name;
: }
This looks like a good candidate for another utility function to keep this one
shorter, e.g. GetRequiredStatsField()?
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-common-test.cc
File be/src/exec/parquet/parquet-common-test.cc:
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-common-test.cc@55
PS9, Line 55: ValidateRanges({{-1, 1}}, 10, {}, false);
I think wrapping this in something called ValidateRangesError() would make it
more readable.
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-common-test.cc@71
PS9, Line 71: result
nit: renaming this to "candidate_pages" would help with readibility
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-common-test.cc@94
PS9, Line 94: ValidatePages({0}, {{0, 9LL + INT_MAX}}, 100LL + INT_MAX, {0});
Maybe add one > UINT_MAX for good measure?
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-common.h
File be/src/exec/parquet/parquet-common.h:
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-common.h@49
PS9, Line 49: if (lhs.first < rhs.first) return true;
I think this is the same as
std::tie(lhs.first, lhs.second) < std::tie(rhs.first, rhs.second)
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/parquet-common.h
File be/src/exec/parquet/parquet-common.h:
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/parquet-common.h@74
PS8, Line 74: can p
> I only like to keep them const to avoid having typos in 'if' statements, e.
I don't feel strongly about it, but it always makes me wonder since we usually
don't denote this in the signature. You could also do if (42 == num_rows) :).
Either way works for me, feel free to leave as is.
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/parquet-common.cc
File be/src/exec/parquet/parquet-common.cc:
http://gerrit.cloudera.org:8080/#/c/12065/8/be/src/exec/parquet/parquet-common.cc@95
PS8, Line 95: DCHECK_LT(page_idx, page_locations.size());
> auto& deduces the const for itself, but I added the const keyword anyway fo
Oh, good to know, thx for the hint.
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-page-index-test.cc
File be/src/exec/parquet/parquet-page-index-test.cc:
http://gerrit.cloudera.org:8080/#/c/12065/9/be/src/exec/parquet/parquet-page-index-test.cc@35
PS9, Line 35: void ConstructFakeRowGroups(const vector&
all_row_group_ranges,
Methods in this file could benefit from
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12282 ) Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2711/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 21:44:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12282 ) Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. Patch Set 6: Code-Review+2 (2 comments) LGTM assuming tests pass. http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/FsType.java File fe/src/main/java/org/apache/impala/catalog/FsType.java: http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/FsType.java@25 PS4, Line 25: > Moved it to FileSystemUtil.java so all the code is in one place. The use of Thanks for the explanation, yeah that makes a lot of sense. http://gerrit.cloudera.org:8080/#/c/12282/4/testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test File testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test: http://gerrit.cloudera.org:8080/#/c/12282/4/testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test@22 PS4, Line 22:S3 partitions=1/0 files=0 size=0B > It's due to the fact that s3_tbl isn't a real table, it's a "test table" cr Oh ok, so that's pre-existing behaviour? I guess that's nbd, although I imagine it will confuse some other people. -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Sahil Takiar Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 21:40:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8363: fix e2e start with impala log dir
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12935 ) Change subject: IMPALA-8363: fix e2e start with impala_log_dir .. Patch Set 1: That would work for me -- To view, visit http://gerrit.cloudera.org:8080/12935 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f46f40474b4b380abe88647a37e8e4d2231d745 Gerrit-Change-Number: 12935 Gerrit-PatchSet: 1 Gerrit-Owner: radford nguyen Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 09 Apr 2019 21:37:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.h@128 PS1, Line 128: bool done_ = false; Can we switch this to an atomic? It'll be easier to reason about the safeness of the "dirty" read. I don't see a real downside to the change. http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc@561 PS1, Line 561: // TODO: Cancelling the RequestContext will wait on in-flight IO. We should > The comment on ScanRange::CancelInternal() claims that it doesn't wait for I think the comment is actually correct - the waiting happens in ScanRange::Cancel(). Unless I missed something. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 21:36:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Sahil Takiar has posted comments on this change. (
http://gerrit.cloudera.org:8080/12282 )
Change subject: IMPALA-6050: Query profiles should indicate storage layer(s)
used
..
Patch Set 4:
(5 comments)
I'm re-running the full suite of tests to ensure there are no test failures
introduced by rebasing this patch. Will post an update when the tests complete.
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/FsType.java
File fe/src/main/java/org/apache/impala/catalog/FsType.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/FsType.java@25
PS4, Line 25: * Represents the type of filesystem being used. Typically
associated with a
> This is somewhat redundant with the is*Filesystem() logic in FileSystemUtil
Moved it to FileSystemUtil.java so all the code is in one place. The use of
FsType is slightly different from the is*FileSystem methods. FsType allows
grouping multiple FileSystems into a single logic type, which is useful for
cloud stores like ADLS because it supports multiple file system connectors:
abfs, abfss, adl. It also also mapping a filesystem scheme to a more user
friendly string - e.g. s3a to S3.
I added some more Javadocs to make it clearer when to use FsType vs. when to
use is*FileSystem.
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@89
PS4, Line 89: return FsType.LOCAL;
> See comment on LocalFsTable
Done
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@276
PS4, Line 276: return FsType.LOCAL;
> Is this right? I think this is a "Local" table in the sense that it's the "
Yeah, thanks for pointing that out. Updated the implementation so its pretty
much the same as what HdfsTable / HdfsPartition does.
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/test/java/org/apache/impala/planner/ExplainTest.java
File fe/src/test/java/org/apache/impala/planner/ExplainTest.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@81
PS4, Line 81:
partitions.add(createMockHdfsPartition("abfs://dummy...@dummy-account.dfs.core"
> This is cool
Thanks :)
http://gerrit.cloudera.org:8080/#/c/12282/4/testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test
File
testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test:
http://gerrit.cloudera.org:8080/#/c/12282/4/testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test@22
PS4, Line 22:S3 partitions=1/0 files=0 size=0B
> What's the deal with the 1/0?
It's due to the fact that s3_tbl isn't a real table, it's a "test table"
created using FrontendTestBase#addTestTable. 't_nopart' in
'insert-sort-by.test' is another example of a table that has the '1/0' behavior.
--
To view, visit http://gerrit.cloudera.org:8080/12282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3
Gerrit-Change-Number: 12282
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Sahil Takiar
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 09 Apr 2019 21:15:16 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Hello Lars Volker, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12282 to look at the new patch set (#6). Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. IMPALA-6050: Query profiles should indicate storage layer(s) used This patch updates Impala explain plans so that the Scan Node section clearly displays which filesystems the Scan Node is reading data from (support has been added for scans from HDFS, S3, ADLS, and the local filesystem). Before this patch, if an Impala query scanned a table with partitions across different storage layers, the explain plan would look like this: PLAN-ROOT SINK | 01:EXCHANGE [UNPARTITIONED] | 00:SCAN HDFS [functional.alltypes] partitions=24/24 files=24 size=478.45KB Now the explain plan will look like this: PLAN-ROOT SINK | 01:EXCHANGE [UNPARTITIONED] | 00:SCAN S3 [functional.alltypes] ADLS partitions=4/24 files=4 size=478.45KB HDFS partitions=10/24 files=10 size=478.45KB S3 partitions=10/24 files=10 size=478.45KB The explain plan differentiates "SCAN HDFS" vs "SCAN S3" by using the root table path. This means that even scans of non-partitioned tables will see their explain plans change from "SCAN HDFS" to "SCAN [storage-layer-name]". This change affects explain plans that are stored on an single storage layer as well: 'partitions=...' will become 'HDFS partitions-...'. This patch makes several changes to PlannerTest.java so that by default test files do not validate the value of the storage layer displayed in the explain plan. This is necessary to support classes such as S3PlannerTest which run test files against S3. It makes several changes to impala_test_suite.py as well in order to support validation of explain plans in test files that run via Python. Specifically, it adds support for a new substitution variable in test files called $FILESYSTEM_NAME which is the name of the storage layer the test is being run against. Testing: * Ran core tests * Added new tests to PlannerTest * Added ExplainTest to allow for more fine-grained testing of explain plan logic Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 --- M fe/pom.xml M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/test/java/org/apache/impala/common/FrontendFixture.java A fe/src/test/java/org/apache/impala/planner/ExplainTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java A testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test M testdata/workloads/functional-query/queries/QueryTest/partition-col-types.test M tests/common/impala_test_suite.py M tests/util/filesystem_utils.py 19 files changed, 624 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/12282/6 -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 6 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12282 )
Change subject: IMPALA-6050: Query profiles should indicate storage layer(s)
used
..
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@772
PS5, Line 772: public SampledPartitionMetadata(long partitionId,
FileSystemUtil.FsType partitionFsType) {
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1323
PS5, Line 1323: for (Map.Entry partsPerFs
: numPartitionsPerFs_.entrySet()) {
line too long (98 > 90)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/test/java/org/apache/impala/planner/ExplainTest.java
File fe/src/test/java/org/apache/impala/planner/ExplainTest.java:
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@99
PS5, Line 99:
partitions.add(createMockHdfsPartition("s3a://dummy-bucket/dummy-part-7",
FileSystemUtil.FsType.S3));
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@100
PS5, Line 100:
partitions.add(createMockHdfsPartition("s3a://dummy-bucket/dummy-part-8",
FileSystemUtil.FsType.S3));
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@101
PS5, Line 101: partitions.add(createMockHdfsPartition(dummyTblPath +
"/dummy-part-9", FileSystemUtil.FsType.HDFS));
line too long (104 > 90)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@102
PS5, Line 102: partitions.add(createMockHdfsPartition(dummyTblPath +
"/dummy-part-10", FileSystemUtil.FsType.HDFS));
line too long (105 > 90)
http://gerrit.cloudera.org:8080/#/c/12282/5/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@148
PS5, Line 148: private HdfsPartition createMockHdfsPartition(String path,
FileSystemUtil.FsType fsType) {
line too long (92 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3
Gerrit-Change-Number: 12282
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 09 Apr 2019 20:59:11 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Hello Lars Volker, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12282 to look at the new patch set (#5). Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. IMPALA-6050: Query profiles should indicate storage layer(s) used This patch updates Impala explain plans so that the Scan Node section clearly displays which filesystems the Scan Node is reading data from (support has been added for scans from HDFS, S3, ADLS, and the local filesystem). Before this patch, if an Impala query scanned a table with partitions across different storage layers, the explain plan would look like this: PLAN-ROOT SINK | 01:EXCHANGE [UNPARTITIONED] | 00:SCAN HDFS [functional.alltypes] partitions=24/24 files=24 size=478.45KB Now the explain plan will look like this: PLAN-ROOT SINK | 01:EXCHANGE [UNPARTITIONED] | 00:SCAN S3 [functional.alltypes] ADLS partitions=4/24 files=4 size=478.45KB HDFS partitions=10/24 files=10 size=478.45KB S3 partitions=10/24 files=10 size=478.45KB The explain plan differentiates "SCAN HDFS" vs "SCAN S3" by using the root table path. This means that even scans of non-partitioned tables will see their explain plans change from "SCAN HDFS" to "SCAN [storage-layer-name]". This change affects explain plans that are stored on an single storage layer as well: 'partitions=...' will become 'HDFS partitions-...'. This patch makes several changes to PlannerTest.java so that by default test files do not validate the value of the storage layer displayed in the explain plan. This is necessary to support classes such as S3PlannerTest which run test files against S3. It makes several changes to impala_test_suite.py as well in order to support validation of explain plans in test files that run via Python. Specifically, it adds support for a new substitution variable in test files called $FILESYSTEM_NAME which is the name of the storage layer the test is being run against. Testing: * Ran core tests * Added new tests to PlannerTest * Added ExplainTest to allow for more fine-grained testing of explain plan logic Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 --- M fe/pom.xml M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/test/java/org/apache/impala/common/FrontendFixture.java A fe/src/test/java/org/apache/impala/planner/ExplainTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java A testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test M testdata/workloads/functional-query/queries/QueryTest/partition-col-types.test M tests/common/impala_test_suite.py M tests/util/filesystem_utils.py 19 files changed, 617 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/12282/5 -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 5 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7971: Add support for insert events in event processor.
Vihang Karajgaonkar has posted comments on this change. (
http://gerrit.cloudera.org:8080/12889 )
Change subject: IMPALA-7971: Add support for insert events in event processor.
..
Patch Set 6:
(15 comments)
Thanks for making the changes. The patch looks great. I have left some minor
suggestions and then its good from my side.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2077
PS6, Line 2077: after inserts
nit, use "after insert events" to be more specific
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@423
PS6, Line 423: refreshCatalogTable
since this method is not doing anything else apart from calling
catalog_refreshTableIfExists .. may be we can get rid of it.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@431
PS6, Line 431: throws CatalogException {
same as above, The method is doing nothing other than calling another method.
So may as well remove it and call the underlying method directly.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@641
PS6, Line 641: List partVals = insertPartition_.getValues();
probably worth while to check the following conditions:
Preconditions.checkNotNull(partVals)
Preconditions.checkState (fsList.size() == partVals.size())
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@649
PS6, Line 649: Automatic
nit, remove Automatic. kind of redundant since debugLog prints the event info
as well.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@673
PS6, Line 673: Automatic
nit, remove
http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3580
PS5, Line 3580: rtition.isMarkedCached())
> This code path is taken by only HDFSTables as you can see on L3522.
Got it. Thanks for the clarification.
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3578
PS6, Line 3578:
filesBeforeInsertForPartitions.put(partition.getId(),
Can we do this conditionally when event processing is enabled?
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3722
PS6, Line 3722: createInsertEventForPartition
nit, Adding plural to the name makes it more readable since it is firing
multiple insert events. may be, createInsertEventsForPartitions?
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3741
PS6, Line 3741: for (String fileName : deltaFiles) {
: newFiles.add(fileName);
: }
Looks like you are making this copy because the InsertEventRequestData in
fireInsertEvent method needs a list. In such a case, perhaps a cleaner way is
to just pass deltaFiles from here and make the copy in that method simply using
rqst.setFilesAdded(new ArrayList<>(deltaFiles))
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3767
PS6, Line 3767: for (String fileName : deltaFiles) {
: newFiles.add(fileName);
: }
same as previous comment, you can just pass the deltaFiles to the
fireInsertEvent and reuse the logic
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@432
PS6, Line 432: public void testInsertEvents() throws TException,
ImpalaException {
thanks for adding this test!
http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@463
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2709/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 20:16:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2710/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 6 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 20:19:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2708/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 4 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 20:10:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2707/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 19:51:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 7 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 19:47:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4003/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 7 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 19:47:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 6 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 19:46:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Austin Nobis has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. IMPALA-8227: Add support for WITH GRANT OPTION with Ranger This patch adds support for the WITH GRANT OPTION SQL syntax when granting privileges to users and groups in Ranger. This allows users who have been granted a privilege to then grant that privilege to other users/groups. Testing: - Ran all FE tests - Ran authorization E2E tests - Added an E2E authorization test in test_ranger to verify the functionality Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 --- M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M tests/authorization/test_ranger.py 2 files changed, 97 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12962/6 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 6 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7981: Add host disk statistics to profile
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12822 ) Change subject: IMPALA-7981: Add host disk statistics to profile .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2706/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I373e7da47a0d722938e6ca1572c49a502951ed57 Gerrit-Change-Number: 12822 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 09 Apr 2019 19:35:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Austin Nobis has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. IMPALA-8227: Add support for WITH GRANT OPTION with Ranger This patch adds support for the WITH GRANT OPTION SQL syntax when granting privileges to users and groups in Ranger. This allows users who have been granted a privilege to then grant that privilege to other users/groups. Testing: - Ran all FE tests - Ran authorization E2E tests - Added an E2E authorization test in test_ranger to verify the functionality Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 --- M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M tests/authorization/test_ranger.py 2 files changed, 90 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12962/5 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12962/4/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/12962/4/tests/authorization/test_ranger.py@168 PS4, Line 168: flake8: W391 blank line at end of file -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 4 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 19:32:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Austin Nobis has posted comments on this change. (
http://gerrit.cloudera.org:8080/12962 )
Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
..
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@138
PS3, Line 138:
> flake8: W291 trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@138
PS3, Line 138: self.execute_query_expect_success(admin_client, "revoke
grant option for select "
> line has trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@159
PS3, Line 159:
> flake8: W291 trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@159
PS3, Line 159: r =
requests.post("{0}/service/xusers/secure/users".format(RANGER_HOST),
> line has trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@168
PS3, Line 168:
> flake8: W391 blank line at end of file
Done
--
To view, visit http://gerrit.cloudera.org:8080/12962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8
Gerrit-Change-Number: 12962
Gerrit-PatchSet: 4
Gerrit-Owner: Austin Nobis
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 09 Apr 2019 19:32:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Austin Nobis has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. IMPALA-8227: Add support for WITH GRANT OPTION with Ranger This patch adds support for the WITH GRANT OPTION SQL syntax when granting privileges to users and groups in Ranger. This allows users who have been granted a privilege to then grant that privilege to other users/groups. Testing: - Ran all FE tests - Ran authorization E2E tests - Added an E2E authorization test in test_ranger to verify the functionality Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 --- M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M tests/authorization/test_ranger.py 2 files changed, 91 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12962/4 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 4 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12962 )
Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
..
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@138
PS3, Line 138:
flake8: W291 trailing whitespace
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@138
PS3, Line 138: self.execute_query_expect_success(admin_client, "revoke
grant option for select "
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@159
PS3, Line 159:
flake8: W291 trailing whitespace
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@159
PS3, Line 159: r =
requests.post("{0}/service/xusers/secure/users".format(RANGER_HOST),
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@168
PS3, Line 168:
flake8: W293 blank line contains whitespace
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@168
PS3, Line 168:
flake8: W391 blank line at end of file
http://gerrit.cloudera.org:8080/#/c/12962/3/tests/authorization/test_ranger.py@168
PS3, Line 168:
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/12962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8
Gerrit-Change-Number: 12962
Gerrit-PatchSet: 3
Gerrit-Owner: Austin Nobis
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 09 Apr 2019 19:29:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7981: Add host disk statistics to profile
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12822 ) Change subject: IMPALA-7981: Add host disk statistics to profile .. Patch Set 6: (1 comment) Rebased the change and made the overall code a bit more consistent. Please see PS6. http://gerrit.cloudera.org:8080/#/c/12822/4/be/src/util/system-state-info.h File be/src/util/system-state-info.h: http://gerrit.cloudera.org:8080/#/c/12822/4/be/src/util/system-state-info.h@255 PS4, Line 255: : : > On the other hand, this one uses standard c++ stringstreams so it might be Went with stringpieces and moved this to an anonymous namespace in the .cc file. -- To view, visit http://gerrit.cloudera.org:8080/12822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I373e7da47a0d722938e6ca1572c49a502951ed57 Gerrit-Change-Number: 12822 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 09 Apr 2019 19:03:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8227: Add support for WITH GRANT OPTION with Ranger
Austin Nobis has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12962 ) Change subject: IMPALA-8227: Add support for WITH GRANT OPTION with Ranger .. IMPALA-8227: Add support for WITH GRANT OPTION with Ranger This patch adds support for the WITH GRANT OPTION SQL syntax when granting privileges to users and groups in Ranger. This allows users who have been granted a privilege to then grant that privilege to other users/groups. Testing: - Ran all FE tests - Ran authorization E2E tests - Added an E2E authorization test in test_ranger to verify the functionality Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 --- M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M tests/authorization/test_ranger.py 2 files changed, 91 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12962/3 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7981: Add host disk statistics to profile
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12822 ) Change subject: IMPALA-7981: Add host disk statistics to profile .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/12822/6/be/src/util/system-state-info-test.cc File be/src/util/system-state-info-test.cc: http://gerrit.cloudera.org:8080/#/c/12822/6/be/src/util/system-state-info-test.cc@122 PS6, Line 122: R"( 8 0 sda 17124835 222797 716029974 8414920 43758807 38432095 7867287712 630179264 0 32547524 638999340 line too long (117 > 90) http://gerrit.cloudera.org:8080/#/c/12822/6/be/src/util/system-state-info-test.cc@123 PS6, Line 123:8 1 sda1 17124482 222797 716027002 8414892 43546943 38432095 7867287712 629089180 0 31590972 637917344)"; line too long (118 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I373e7da47a0d722938e6ca1572c49a502951ed57 Gerrit-Change-Number: 12822 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 09 Apr 2019 19:04:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7981: Add host disk statistics to profile
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12822 to look at the new patch set (#6). Change subject: IMPALA-7981: Add host disk statistics to profile .. IMPALA-7981: Add host disk statistics to profile This change adds host disk statistics to profiles. For each host that participates in the query execution it adds the read and write bandwidth across all disks. This includes all data read or written by the host as part of the execution of a query (spilling), by the HDFS data node, and by other processes running on the same system. The change adds tests for the added functionality to the backend and end to end tests. Change-Id: I373e7da47a0d722938e6ca1572c49a502951ed57 --- M be/src/runtime/query-state.cc M be/src/util/disk-info.h M be/src/util/system-state-info-test.cc M be/src/util/system-state-info.cc M be/src/util/system-state-info.h M tests/query_test/test_observability.py 6 files changed, 257 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/12822/6 -- To view, visit http://gerrit.cloudera.org:8080/12822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I373e7da47a0d722938e6ca1572c49a502951ed57 Gerrit-Change-Number: 12822 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-8363: fix e2e start with impala log dir
radford nguyen has posted comments on this change. ( http://gerrit.cloudera.org:8080/12935 ) Change subject: IMPALA-8363: fix e2e start with impala_log_dir .. Patch Set 1: > Patch Set 1: > > (1 comment) I think the kwargs approach is better because then `start_impala_cluster` can still be "in charge" of defaulting the `impala_log_dir` option -- To view, visit http://gerrit.cloudera.org:8080/12935 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f46f40474b4b380abe88647a37e8e4d2231d745 Gerrit-Change-Number: 12935 Gerrit-PatchSet: 1 Gerrit-Owner: radford nguyen Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: radford nguyen Gerrit-Comment-Date: Tue, 09 Apr 2019 18:44:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2705/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 18:11:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/12282 )
Change subject: IMPALA-6050: Query profiles should indicate storage layer(s)
used
..
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/FsType.java
File fe/src/main/java/org/apache/impala/catalog/FsType.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/FsType.java@25
PS4, Line 25: * Represents the type of filesystem being used. Typically
associated with a
This is somewhat redundant with the is*Filesystem() logic in
FileSystemUtil.java. Maybe we should move this enum there so that it's easier
to find the relate code. Or cross-reference it? Or share some of the
implementation?
Mainly just concerned that this is another place people have to update to add a
filesystem. There's a more theoretical concern that maybe multiple URL schemes
map to a single filesystem implementation, so the other code could recognise a
filesystem that this doesn't. That seems far-fetched though.
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@89
PS4, Line 89: return FsType.LOCAL;
See comment on LocalFsTable
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@276
PS4, Line 276: return FsType.LOCAL;
Is this right? I think this is a "Local" table in the sense that it's the
"Local" catalog implementation. But it could be any table.
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/test/java/org/apache/impala/planner/ExplainTest.java
File fe/src/test/java/org/apache/impala/planner/ExplainTest.java:
http://gerrit.cloudera.org:8080/#/c/12282/4/fe/src/test/java/org/apache/impala/planner/ExplainTest.java@81
PS4, Line 81:
partitions.add(createMockHdfsPartition("abfs://dummy...@dummy-account.dfs.core"
This is cool
http://gerrit.cloudera.org:8080/#/c/12282/4/testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test
File
testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test:
http://gerrit.cloudera.org:8080/#/c/12282/4/testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test@22
PS4, Line 22:S3 partitions=1/0 files=0 size=0B
What's the deal with the 1/0?
--
To view, visit http://gerrit.cloudera.org:8080/12282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3
Gerrit-Change-Number: 12282
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 09 Apr 2019 18:19:46 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12962 ) Change subject: [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2704/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 2 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 18:01:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8338 : Check CREATION TIME of databases in event processor to avoid incorrect/redundant invalidates
Vihang Karajgaonkar has posted comments on this change. (
http://gerrit.cloudera.org:8080/12938 )
Change subject: IMPALA-8338 : Check CREATION_TIME of databases in event
processor to avoid incorrect/redundant invalidates
..
Patch Set 4:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@8
PS4, Line 8: incorrect/redundant invalidates
I think this is not accurate. creation_time is not used for invalidating but
rather for checking if the exact same instance of Db exists before dropping. So
the a more apt description should be "... to avoid incorrect delete of
databases"
http://gerrit.cloudera.org:8080/#/c/12938/4//COMMIT_MSG@15
PS4, Line 15: we do not need
: to invalidate it in this case.
suggest you to change it to ".. and we can skip the event instead" which is
more accurate.
http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h
File be/src/util/event-metrics.h:
http://gerrit.cloudera.org:8080/#/c/12938/4/be/src/util/event-metrics.h@40
PS4, Line 40: static IntCounter* NUM_EVENTS_FILTERED_COUNTER;
Is there any specific value in exposing this metric on the Catalog metrics
page? The end-user most likely does not care about this metric or can take any
decisions based on this metric. If this metric is only to improve debugging and
observability of this feature then I would suggest it to keep it visible only
in the /events page which gives more detailed view of the event processor.
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@953
PS4, Line 953: public Integer getDbCreationTime(String dbName) {
like suggested in my other comment related to race conditions , this API in
itself is not very useful since no action can be taken based on creationTime
itself without exposing races. May be change this to removeDbIfExists(Database
msDb) which checks the creationTime and removes if it matches.
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@208
PS4, Line 208:
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC).inc(numFilteredEvents);
> line too long (98 > 90)
Just having this line is sufficient to expose the metric in /events page. You
don't need the other changes in the event-metrics.h etc if you decide to skip
exposing this metric to the metrics page.
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@898
PS4, Line 898: the latest and we do not need to invalidate it in this case
The process method does not invalidate so this description needs to be updated.
suggest you to change it "... in the catalog is a later version and we can skip
the event"
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@903
PS4, Line 903: int dbCreationTimeFromEvent = droppedDatabase_.getCreateTime();
: Integer dbCreationTimeFromCatalog =
catalog_.getDbCreationTime(dbName_);
:
: if (dbCreationTimeFromCatalog == null) {
: infoLog("Database {} not present in the catalog, hence
no " +
: "action is required for DROP_DATABASE event.",
dbName_);
: return;
: }
:
: if (dbCreationTimeFromCatalog > dbCreationTimeFromEvent) {
: infoLog("Not removing database {} from catalog as it
already has a" +
: " DB object which was created later.", dbName_);
:
metrics_.getCounter(MetastoreEventsProcessor.EVENTS_FILTERED_METRIC).inc();
This code has potential race conditions. For example, the createTime of the
database which is fetched on line 903 could change by the time it is checked at
line 912. Both the creation_time check and database removal should be done
atomically while holding the write lock. See
CatalogServiceCatalog#removeTableIfExists as an example.
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File
fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12938/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@272
PS4, Line 272: public void t
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 7: Code-Review+2 (2 comments) Carry Bharath's +2. http://gerrit.cloudera.org:8080/#/c/12963/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12963/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@69 PS5, Line 69: private final FeCatalogManager catalog_; : private final Supplier you could get rid of this using JniUtil. serializeToThrift(input) ? Ah, yuo're right. Done. http://gerrit.cloudera.org:8080/#/c/12963/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@238 PS5, Line 238: ng_user(user); > nit: call it validateSentryAdmin...() ? Since it validates and throws.. Done -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 17:27:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7031: Cancel action of debug page should not unregister query
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12926 ) Change subject: IMPALA-7031: Cancel action of debug page should not unregister query .. Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/12926/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12926/4//COMMIT_MSG@9 PS4, Line 9: When a running query is cancelled from the Cancel button of debug nit: queries debug page http://gerrit.cloudera.org:8080/#/c/12926/4//COMMIT_MSG@10 PS4, Line 10: But the client : is unaware of the query is unregistered, it will get error/exception : on other attempts This can result in a scenario where a client that has a reference to the query handle is unaware that the query has been unregistered via the debug page, and attempts to run an operation on that query handle but encounters and error instead. http://gerrit.cloudera.org:8080/#/c/12926/4//COMMIT_MSG@12 PS4, Line 12: The patch updated the ImpalaHttpHandler to make : "Cancel" button will only cancel the query instead of : unregister it This patch updates the cancel http endpoint to only cancel the query. http://gerrit.cloudera.org:8080/#/c/12926/4//COMMIT_MSG@18 PS4, Line 18: Run nit: ran http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py@537 PS4, Line 537: Test if unregister query when press "Cancel" button on : the debug page (impalad WebUI) Verify that the cancel http endpoint does not unregister the query http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py@540 PS4, Line 540: cancel cancels http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py@547 PS4, Line 547: assert "Query cancellation successful" in responses[0].text assert "Query cancellation successful" in responses[0].text, responses[0].text http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py@548 PS4, Line 548: # After the query is successfully canceled, the client runs a get_state operation to verify that the query id is still registered http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py@551 PS4, Line 551: unregistered nit not necessarily the case, could be anything. Just say something like "Encountered unexpected exception. Query might be unregisted already. Exception: " http://gerrit.cloudera.org:8080/#/c/12926/4/tests/webserver/test_web_pages.py@552 PS4, Line 552: finally: self.client.close(query_handle) -- To view, visit http://gerrit.cloudera.org:8080/12926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56983d40e0542bc734ec5a66c339b5131b7b56c8 Gerrit-Change-Number: 12926 Gerrit-PatchSet: 4 Gerrit-Owner: Alice Fan Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 17:35:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12962 )
Change subject: [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger
..
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/12962/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12962/2//COMMIT_MSG@7
PS2, Line 7: [IMPALA-8227]
nit: use IMPALA-8227: instead
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py
File tests/authorization/test_ranger.py:
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@119
PS2, Line 119: time.sleep(35)
nit: put # TODO: IMPALA-8293 use refresh authorization
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@119
PS2, Line 119: time.sleep(35)
: self.execute_query_expect_success(user1_client,
: "grant select on
database {0} to user {1}"
: .format(unique_database,
user2), user=user1)
: # TODO: IMPALA-8293 use refresh authorization
: time.sleep(35)
: self.execute_query_expect_success(user2_client, "show
tables in {0}"
:
.format(unique_database), user=user2)
: self.execute_query_expect_success(user1_client,
: "revoke select on
database {0} from user "
:
"{1}".format(unique_database, user2), user=user1)
: # TODO: IMPALA-8293 use refresh authorization
: time.sleep(35)
shouldn't all these sleeps be 10 seconds?
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@132
PS2, Line 132: self.execute_query_expect_failure(user2_client, "show
tables in {0}"
: .format(unique_database))
can we have a test case for "revoke grant option" here and verify it?
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@147
PS2, Line 147: http://localhost:6080
make the ranger admin host as a global variable, i.e. RANGER_ADMIN_HOST
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@148
PS2, Line 148: auth=("admin", "admin")
nit: make this a global variable so that it's available to other functions,
i.e. RANGER_AUTH
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@148
PS2, Line 148: auth=("admin", "admin"),
: json=data, headers=headers)
nit: fix indentation
http://gerrit.cloudera.org:8080/#/c/12962/2/tests/authorization/test_ranger.py@143
PS2, Line 143: def _add_ranger_user(self, user):
: data = {"name": user, "password": "password123",
"userRoleList": ["ROLE_USER"]}
: headers = {"Content-Type": "application/json", "Accept":
"application/json"}
:
: r =
requests.post("http://localhost:6080/service/xusers/secure/users";,
: auth=("admin", "admin"),
: json=data, headers=headers)
: return json.loads(r.content)['id']
:
: def _remove_ranger_user(self, id):
:
requests.delete("http://localhost:6080/service/xusers/users/{0}?forceDelete=true";
: .format(id), auth=("admin", "admin"))
we should check for status code and return an error when it's not 2XX.
--
To view, visit http://gerrit.cloudera.org:8080/12962
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8
Gerrit-Change-Number: 12962
Gerrit-PatchSet: 2
Gerrit-Owner: Austin Nobis
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Tue, 09 Apr 2019 17:35:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 17:28:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4002/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 17:28:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. IMPALA-8385: Refactor Sentry admin user check This patch moves the hardcoded Sentry admin user check from the generic "show roles" and "show grant" catalog operations to Sentry authorization plugin, i.e. SentryImpaladAuthorizationManager. This patch also removes isAdmin() from AuthorizationManager interface. Testing: - Added a new authorization E2E test to test for Sentry admin check - Ran all FE tests - Ran all authorization E2E tests Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 --- M be/src/catalog/catalog-server.cc M be/src/catalog/catalog.cc M be/src/catalog/catalog.h M be/src/exec/catalog-op-executor.cc M be/src/exec/catalog-op-executor.h M be/src/service/client-request-state.cc M be/src/service/fe-support.cc M common/thrift/CatalogService.thrift M fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A tests/authorization/test_sentry.py 17 files changed, 194 insertions(+), 82 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/12963/7 -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger
Austin Nobis has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12962 Change subject: [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger .. [IMPALA-8227] Add support for WITH GRANT OPTION with Ranger This patch adds support for the WITH GRANT OPTION SQL syntax when granting privileges to users and groups in Ranger. This allows users who have been granted a privilege to then grant that privilege to other users/groups. Testing: - Ran all FE tests - Ran authorization E2E tests - Added an E2E authorization test in test_ranger to verify the functionality Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 --- M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M tests/authorization/test_ranger.py 2 files changed, 77 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/12962/2 -- To view, visit http://gerrit.cloudera.org:8080/12962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9c2384f0a9fe30bea1eaceac5b27b1c432383aa8 Gerrit-Change-Number: 12962 Gerrit-PatchSet: 2 Gerrit-Owner: Austin Nobis
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2703/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 17:15:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12927 ) Change subject: IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2702/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If46b4bf24d916e4a4ea8a36ff4acfd95d5f45c8e Gerrit-Change-Number: 12927 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 09 Apr 2019 16:58:24 + Gerrit-HasComments: No
[Impala-ASF-CR] This change bumps the CDH BUILD VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for t
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12965 ) Change subject: This change bumps the CDH_BUILD_VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for the upcoming patch for IMPALA-8338. .. Patch Set 1: oops, raced with Fredy! -- To view, visit http://gerrit.cloudera.org:8080/12965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id38bae921c4d93421c6c72cdccef6a4783e2588e Gerrit-Change-Number: 12965 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 09 Apr 2019 16:55:15 + Gerrit-HasComments: No
[Impala-ASF-CR] This change bumps the CDH BUILD VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for t
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12965 ) Change subject: This change bumps the CDH_BUILD_VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for the upcoming patch for IMPALA-8338. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG@6 PS1, Line 6: missed this, can you please add a one liner commit title, something like this? https://github.com/apache/impala/commit/88485880a5e709d78d3a51324448b0f11ba71009 -- To view, visit http://gerrit.cloudera.org:8080/12965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id38bae921c4d93421c6c72cdccef6a4783e2588e Gerrit-Change-Number: 12965 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 09 Apr 2019 16:54:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] This change bumps the CDH BUILD VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for t
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12965 ) Change subject: This change bumps the CDH_BUILD_VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for the upcoming patch for IMPALA-8338. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG@7 PS1, Line 7: This change bumps the CDH_BUILD_VERSION nit: do you mind adding a subject to make it more aligned with git commit style? Bump CDH_BUILD_NUMBER to 1009254 -- To view, visit http://gerrit.cloudera.org:8080/12965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id38bae921c4d93421c6c72cdccef6a4783e2588e Gerrit-Change-Number: 12965 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 09 Apr 2019 16:54:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] This change bumps the CDH BUILD VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for t
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12965 ) Change subject: This change bumps the CDH_BUILD_VERSION to a version that includes the change to have full-thrift object for DropDatabaseMessage in Metastore notifications. This change is needed for the upcoming patch for IMPALA-8338. .. Patch Set 1: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG@9 PS1, Line 9: s. Can you mention the hive jira, if there is one? http://gerrit.cloudera.org:8080/#/c/12965/1//COMMIT_MSG@11 PS1, Line 11: Mind adding a short note on testing you did to validate these bits? -- To view, visit http://gerrit.cloudera.org:8080/12965 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id38bae921c4d93421c6c72cdccef6a4783e2588e Gerrit-Change-Number: 12965 Gerrit-PatchSet: 1 Gerrit-Owner: Bharath Krishna Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Tue, 09 Apr 2019 16:52:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 6: Code-Review+2 (2 comments) lgtm. http://gerrit.cloudera.org:8080/#/c/12963/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12963/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@69 PS5, Line 69: private final static TBinaryProtocol.Factory THRIFT_PROTOCOL = : new TBinaryProtocol.Factory(); you could get rid of this using JniUtil. serializeToThrift(input) ? http://gerrit.cloudera.org:8080/#/c/12963/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@238 PS5, Line 238: checkSentryAdmin nit: call it validateSentryAdmin...() ? Since it validates and throws.. -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 16:48:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/12963/5/tests/authorization/test_sentry.py File tests/authorization/test_sentry.py: http://gerrit.cloudera.org:8080/#/c/12963/5/tests/authorization/test_sentry.py@39 PS5, Line 39: non_admin = unique_name > Consider using unique_name instead of "foobar" Done -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 16:36:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12968/1/be/src/exec/hdfs-scan-node.cc@561 PS1, Line 561: // TODO: Cancelling the RequestContext will wait on in-flight IO. We should The comment on ScanRange::CancelInternal() claims that it doesn't wait for in-flight IO. Does it need to be updated? -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 16:39:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12282 ) Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2701/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Comment-Date: Tue, 09 Apr 2019 16:37:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. IMPALA-8385: Refactor Sentry admin user check This patch moves the hardcoded Sentry admin user check from the generic "show roles" and "show grant" catalog operations to Sentry authorization plugin, i.e. SentryImpaladAuthorizationManager. This patch also removes isAdmin() from AuthorizationManager interface. Testing: - Added a new authorization E2E test to test for Sentry admin check - Ran all FE tests - Ran all authorization E2E tests Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 --- M be/src/catalog/catalog-server.cc M be/src/catalog/catalog.cc M be/src/catalog/catalog.h M be/src/exec/catalog-op-executor.cc M be/src/exec/catalog-op-executor.h M be/src/service/client-request-state.cc M be/src/service/fe-support.cc M common/thrift/CatalogService.thrift M fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java M fe/src/main/java/org/apache/impala/service/FeSupport.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java A tests/authorization/test_sentry.py 17 files changed, 197 insertions(+), 82 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/12963/6 -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12927 )
Change subject: IMPALA-8363: Deny access when column masking or row filtering
is enabled in Ranger
..
Patch Set 6:
(7 comments)
The new implementation based on the work in this CR:
https://gerrit.cloudera.org/c/12959/
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@106
PS4, Line 106:
> consider adding @Nullable for this parameter
Done. StatementBase is no longer needed.
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@120
PS4, Line 120:
> can you add to the javadoc indicating what 'statement' is used for
Same as above.
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@123
PS4, Line 123: y do, thr
> and this one
Sane as above.
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java
File
fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/NoopAuthorizationFactory.java@47
PS4, Line 47:
> if it's not too much of a pain, is it possible to separate out these unrela
Done
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
File
fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@160
PS4, Line 160: if (request.getAuthorizable().getType() == Type.COLUMN) {
> I think part of the problem here is column-level permissions on views are b
I changed the implementation based on https://gerrit.cloudera.org/c/12959/.
Please take a look at it again.
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@206
PS4, Line 206:* enabled but not in Impala.
> Curious whether it would be feasible to give a different error message indi
Done
http://gerrit.cloudera.org:8080/#/c/12927/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@250
PS4, Line 250: accessType = RangerPolicyEngine.ANY_ACCESS;
> I'm worried that this pattern will be very hard to miss when adding a new t
No longer needed.
--
To view, visit http://gerrit.cloudera.org:8080/12927
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If46b4bf24d916e4a4ea8a36ff4acfd95d5f45c8e
Gerrit-Change-Number: 12927
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 09 Apr 2019 16:33:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12927 ) Change subject: IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger .. IMPALA-8363: Deny access when column masking or row filtering is enabled in Ranger This patch updates the Ranger authorization checker code to deny access when column masking and row filtering is enabled in Ranger for queries that that have columns/tables specified in column mask and row filter policies. This is to prevent data leak, such that the data that is masked/filtered in Hive should not be visible at all in Impala until Impala has full support for column masking and row filtering. Testing: - Added tests in AuthorizationStmtTest to test queries with column masking and row filtering enabled. - Ran all FE tests - Ran all E2E tests Change-Id: If46b4bf24d916e4a4ea8a36ff4acfd95d5f45c8e --- M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java 11 files changed, 379 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/12927/6 -- To view, visit http://gerrit.cloudera.org:8080/12927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If46b4bf24d916e4a4ea8a36ff4acfd95d5f45c8e Gerrit-Change-Number: 12927 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 ) Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2700/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 16:30:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6050: Query profiles should indicate storage layer(s) used
Hello Lars Volker, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12282 to look at the new patch set (#4). Change subject: IMPALA-6050: Query profiles should indicate storage layer(s) used .. IMPALA-6050: Query profiles should indicate storage layer(s) used This patch updates Impala explain plans so that the Scan Node section clearly displays which filesystems the Scan Node is reading data from (support has been added for scans from HDFS, S3, ADLS, and the local filesystem). Before this patch, if an Impala query scanned a table with partitions across different storage layers, the explain plan would look like this: PLAN-ROOT SINK | 01:EXCHANGE [UNPARTITIONED] | 00:SCAN HDFS [functional.alltypes] partitions=24/24 files=24 size=478.45KB Now the explain plan will look like this: PLAN-ROOT SINK | 01:EXCHANGE [UNPARTITIONED] | 00:SCAN S3 [functional.alltypes] ADLS partitions=4/24 files=4 size=478.45KB HDFS partitions=10/24 files=10 size=478.45KB S3 partitions=10/24 files=10 size=478.45KB The explain plan differentiates "SCAN HDFS" vs "SCAN S3" by using the root table path. This means that even scans of non-partitioned tables will see their explain plans change from "SCAN HDFS" to "SCAN [storage-layer-name]". This change affects explain plans that are stored on an single storage layer as well: 'partitions=...' will become 'HDFS partitions-...'. This patch makes several changes to PlannerTest.java so that by default test files do not validate the value of the storage layer displayed in the explain plan. This is necessary to support classes such as S3PlannerTest which run test files against S3. It makes several changes to impala_test_suite.py as well in order to support validation of explain plans in test files that run via Python. Specifically, it adds support for a new substitution variable in test files called $FILESYSTEM_NAME which is the name of the storage layer the test is being run against. Testing: * Ran core tests * Added new tests to PlannerTest * Added ExplainTest to allow for more fine-grained testing of explain plan logic Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 --- M fe/pom.xml M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/catalog/FeFsPartition.java M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java A fe/src/main/java/org/apache/impala/catalog/FsType.java M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java M fe/src/test/java/org/apache/impala/common/FrontendFixture.java A fe/src/test/java/org/apache/impala/planner/ExplainTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java M fe/src/test/java/org/apache/impala/testutil/TestUtils.java A testdata/workloads/functional-planner/queries/PlannerTest/scan-node-fs-scheme.test M testdata/workloads/functional-query/queries/QueryTest/partition-col-types.test M tests/common/impala_test_suite.py M tests/util/filesystem_utils.py 19 files changed, 618 insertions(+), 87 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/12282/4 -- To view, visit http://gerrit.cloudera.org:8080/12282 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b1b4a1bc1a24e9614e3b4dc5a61dc96d075d1c3 Gerrit-Change-Number: 12282 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-8322: Add periodic dirty check of done in ThreadTokenAvailableCb
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12968 Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb .. IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb When HdfsScanNode is cancelled or hits and error, SetDoneInternal() holds HdfsScanNode::lock_ while it runs RequestContext::Cancel(), which can wait on IO threads to complete outstanding IOs. This can cause a cascade of blocked threads that causes Prepare() to take a significant time and cause datastream sender timeouts. The specific scenario seen has this set of threads: Thread 1: A DiskIoMgr thread is blocked on IO in hdfsOpenFile() or hdfsRead(), holding HdfsFileReader::lock_. Thread 2: An HDFS scanner thread is blocked in HdfsScanNode::SetDoneInternal() -> RequestContext::Cancel() -> ScanRange::CancelInternal(), waiting on HdfsFileReader::lock_. It is holding HdfsScanNode::lock_. Thread 3: A thread in ThreadResourceMgr::DestroyPool() -> (a few layers) -> HdfsScanNode::ThreadTokenAvailableCb() is blocked waiting on HdfsScanNode::lock_ while holding ThreadResourceMgr::lock_. Thread 4: A thread in FragmentInstanceState::Prepare() -> RuntimeState::Init() -> ThreadResourceMgr::CreatePool() is blocked waiting on ThreadResourceMgr::lock_. When Prepare() takes a significant time, datastream senders will time out waiting for the datastream receivers to start up. This causes failed queries. S3 has higher latencies for IO and does not have file handle caching, so S3 is more susceptible to this issue than other platforms. This changes HdfsScanNode::ThreadTokenAvailableCb() to periodically do a dirty check of HdfsScanNode::done_ when waiting to acquire the lock. This avoids the blocking experienced by Thread 3 in the example above. Testing: - Ran tests on normal HDFS and repeatedly on S3 Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 --- M be/src/common/names.h M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scan-node.h 3 files changed, 28 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/12968/1 -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 1 Gerrit-Owner: Joe McDonnell
[Impala-ASF-CR] IMPALA-8385: Refactor Sentry admin user check
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12963 ) Change subject: IMPALA-8385: Refactor Sentry admin user check .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12963/5/tests/authorization/test_sentry.py File tests/authorization/test_sentry.py: http://gerrit.cloudera.org:8080/#/c/12963/5/tests/authorization/test_sentry.py@39 PS5, Line 39: non_admin = "foobar" Consider using unique_name instead of "foobar" -- To view, visit http://gerrit.cloudera.org:8080/12963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I911228b09af7eed5d5dc002b20591ef64dc625d3 Gerrit-Change-Number: 12963 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 09 Apr 2019 15:36:19 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8371: Return appropriate error code for unified backend tests
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12885 ) Change subject: IMPALA-8371: Return appropriate error code for unified backend tests .. IMPALA-8371: Return appropriate error code for unified backend tests Unified backend tests rely on generating bash scripts for each test that call the unified executable with a filter to run the appropriate subset of the tests. The generated script currently does not return the return code from the test execution. This changes the test execution scripts to return the appropriate return code. To do this, the script generator is changed from a bash implementation in bin/gen-backend-test-script.sh to a python implementation in bin/gen-backend-test-script.py. This makes it easier to handle shell variables in the script template correctly. Testing: - Ran backend tests on centos 6, centos 7 - Manually tested with a failing test and verified return value Change-Id: Ia146d026d42f76d5ea12d92798f299182de03eef Reviewed-on: http://gerrit.cloudera.org:8080/12885 Reviewed-by: Joe McDonnell Tested-by: Impala Public Jenkins --- M be/CMakeLists.txt A bin/gen-backend-test-script.py D bin/gen-backend-test-script.sh 3 files changed, 80 insertions(+), 51 deletions(-) Approvals: Joe McDonnell: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12885 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia146d026d42f76d5ea12d92798f299182de03eef Gerrit-Change-Number: 12885 Gerrit-PatchSet: 7 Gerrit-Owner: Joe McDonnell Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-7322: Add storage wait time to profile
Yongzhi Chen has posted comments on this change. (
http://gerrit.cloudera.org:8080/12940 )
Change subject: IMPALA-7322: Add storage wait time to profile
..
Patch Set 3:
(4 comments)
Will fix the names, please see my responses for other comments.
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java:
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@232
PS3, Line 232: loadedTbls_
> How do you distinguish between tables loaded now vs tables already loaded.
See line 234:
requestedTbls.contains(((Table)ldTbl).getTableName()))
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@233
PS3, Line 233: ldTbl instanceof Table
> This is kinda obvious, force a downcast?
FeTable is a interface, Table just implement it, so not all FeTable is Table.
So I think it is necessary.
private final Map loadedTbls_ = new HashMap<>();
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
File fe/src/main/java/org/apache/impala/catalog/HBaseTable.java:
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HBaseTable.java@109
PS3, Line 109: hbaseTableName_ = Util.getHBaseTableName(getMetaStoreTable());
: // Warm up the connection and verify the table exists.
: Util.getHBaseTable(hbaseTableName_).close();
: columnFamilies_ = null;
: cols = Util.loadColumns(msTable_);
: } finally {
: storageLdTime_ = ctxStg.stop();
> I don't think any of this calls into HBase
Util.getHBaseTable(hbaseTableName_).close();
is the only call in load method that calls hbase.
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:
http://gerrit.cloudera.org:8080/#/c/12940/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1237
PS3, Line 1237: final Timer.Context ctxStg =
> I think the scopes for metrics are incorrect. I'd suggest to do this on top
Do not understand your comment.
--
To view, visit http://gerrit.cloudera.org:8080/12940
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6dde7e394b7c1c396d835ef6aa0a55930c0a8660
Gerrit-Change-Number: 12940
Gerrit-PatchSet: 3
Gerrit-Owner: Yongzhi Chen
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Reviewer: Yongzhi Chen
Gerrit-Comment-Date: Tue, 09 Apr 2019 14:09:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Add impala group.json to .gitignore
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12964 ) Change subject: Add impala_group.json to .gitignore .. Add impala_group.json to .gitignore This patch adds ranger/setup/impala_group.json into .gitignore. Change-Id: I20ab5080fe931ac37af2403d7b85adb78916660a Reviewed-on: http://gerrit.cloudera.org:8080/12964 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M testdata/cluster/.gitignore 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I20ab5080fe931ac37af2403d7b85adb78916660a Gerrit-Change-Number: 12964 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Add impala group.json to .gitignore
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12964 ) Change subject: Add impala_group.json to .gitignore .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I20ab5080fe931ac37af2403d7b85adb78916660a Gerrit-Change-Number: 12964 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 09 Apr 2019 10:37:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8395: Parse older formats of /proc/net/dev correctly
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12954 ) Change subject: IMPALA-8395: Parse older formats of /proc/net/dev correctly .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic804955d8e4269e787037a6dc68bef2d70382426 Gerrit-Change-Number: 12954 Gerrit-PatchSet: 5 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Comment-Date: Tue, 09 Apr 2019 10:31:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8395: Parse older formats of /proc/net/dev correctly
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12954 ) Change subject: IMPALA-8395: Parse older formats of /proc/net/dev correctly .. IMPALA-8395: Parse older formats of /proc/net/dev correctly Older kernel versions don't have a space between the interface name and the first counter value in /proc/net/dev. This change reworks the parsing logic to support such older formats and adds a unit test for it. Change-Id: Ic804955d8e4269e787037a6dc68bef2d70382426 Reviewed-on: http://gerrit.cloudera.org:8080/12954 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/util/system-state-info-test.cc M be/src/util/system-state-info.cc M be/src/util/system-state-info.h 3 files changed, 46 insertions(+), 11 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12954 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic804955d8e4269e787037a6dc68bef2d70382426 Gerrit-Change-Number: 12954 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho
