You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org> on 2021/08/26 22:58:53 UTC
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17812
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
IMPALA-10888: getPartitionsByNames should return sorted partitions
The catalog's HMS API implementation for getPartitionsByNames
currently does not return partitions in any particular order.
This can cause test flakiness where some tests expect partitions
in a given order. Instead of modifying the tests this change
changes the implementation of the API such that it always returns
the partitions sorted by partition name. This is in-line with
how hive metastore returns the partitions over this API.
Testing:
1. Added a new unit test and modified existing test
to assert the sorted order.
Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
---
M fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
M fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java
2 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/17812/1
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 22:49:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 4
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 22:54:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
kishen@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 22:31:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9391/ : 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/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 22:52:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9379/ : 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/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Aug 2021 23:21:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 05:05:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Aman Sinha (Code Review)" <ge...@cloudera.org>.
Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@463
PS2, Line 463: (part) -> MetastoreShim.makePartName(partitionColNames, part.getValues())));
Is there a way to do this only for the tests to avoid sorting potentially large number of partition names under normal operation ?
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 17:35:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@463
PS2, Line 463: (part) -> MetastoreShim.makePartName(partitionColNames, part.getValues())));
> Yes, I had initially thought of making this test only change. However, I th
Uploaded a new patch which modifies the tests instead of the API.
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 22:28:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
IMPALA-10888: getPartitionsByNames should return sorted partitions
The catalog's HMS API implementation for getPartitionsByNames
currently does not return partitions in any particular order.
This can cause test flakiness where some tests expect partitions
in a given order. Instead of modifying the tests this change
changes the implementation of the API such that it always returns
the partitions sorted by partition name. This is in-line with
how hive metastore returns the partitions over this API.
Testing:
1. Added a new unit test and modified existing test
to assert the sorted order.
Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
---
M fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
M fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java
2 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/17812/2
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17812/1/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/1/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@445
PS1, Line 445: sortPartitionsByNames(response.table_info.hms_table.getPartitionKeys(), retPartitions);
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Aug 2021 22:59:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
IMPALA-10888: sort partitions by name before comparing in test
CatalogHmsFileMetadataTest and EnableCatalogdHmsCacheFlagTest
compare the partition filemetada which is returned over by
get_partitions_by_names HMS API. However, the catalogd's
implementation of API does not return the partitions in a
deterministic order. Hence the tests can fail intermittently.
The patch sorts the returned partitions by names before
making any comparisons to avoid the flakiness of the test.
Testing:
1. Modified existing test to do the sorting before
asserting the filemetadata comparison.
Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
---
M fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
M fe/src/test/java/org/apache/impala/catalog/metastore/AbstractCatalogMetastoreTest.java
M fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java
M fe/src/test/java/org/apache/impala/catalog/metastore/EnableCatalogdHmsCacheFlagTest.java
4 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/17812/3
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 3
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Sourabh Goyal (Code Review)" <ge...@cloudera.org>.
Sourabh Goyal has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@459
PS2, Line 459: for (FieldSchema partSchema : partitionKeys) {
Assuming retPartitions list is non empty, should we assert that partitionKeys.size() == retPartitions.get(0).getValues().size() ?
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 17:24:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7432/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 4
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 22:54:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7425/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Aug 2021 22:59:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17812/1/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/1/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@445
PS1, Line 445: sortPartitionsByNames(response.table_info.hms_table.getPartitionKeys(), retPartitions);
> line too long (91 > 90)
Done
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Aug 2021 23:01:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
IMPALA-10888: sort partitions by name before comparing in test
CatalogHmsFileMetadataTest and EnableCatalogdHmsCacheFlagTest
compare the partition filemetada which is returned over by
get_partitions_by_names HMS API. However, the catalogd's
implementation of API does not return the partitions in a
deterministic order. Hence the tests can fail intermittently.
The patch sorts the returned partitions by names before
making any comparisons to avoid the flakiness of the test.
Testing:
1. Modified existing test to do the sorting before
asserting the filemetadata comparison.
Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Reviewed-on: http://gerrit.cloudera.org:8080/17812
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
M fe/src/test/java/org/apache/impala/catalog/metastore/AbstractCatalogMetastoreTest.java
M fe/src/test/java/org/apache/impala/catalog/metastore/CatalogHmsFileMetadataTest.java
M fe/src/test/java/org/apache/impala/catalog/metastore/EnableCatalogdHmsCacheFlagTest.java
4 files changed, 42 insertions(+), 0 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@459
PS2, Line 459: for (FieldSchema partSchema : partitionKeys) {
> Assuming retPartitions list is non empty, should we assert that partitionKe
Yes, I think that is a valid preconditions check given that this is a public static method and not a private method.
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@463
PS2, Line 463: (part) -> MetastoreShim.makePartName(partitionColNames, part.getValues())));
> If this is done only for testing. We can just have a helper utility method,
Yes, I had initially thought of making this test only change. However, I thought it may be a better solution to simulate what the HMS is doing since we cannot be sure if HMS clients assume sorted order or not in the returned partitions. Currently, the order of partitions returned in case of a catalogd HMS call v/s direct HMS call is not the same which feels like a discrepancy to me.
I guess we can keep the current implementation and modify the test until we see a real-world case where clients assume sorted partitions. Thoughts?
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 19:53:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10888: sort partitions by name before comparing in test
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: sort partitions by name before comparing in test
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 4
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 28 Aug 2021 05:05:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9380/ : 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/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Aug 2021 23:23:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10888: getPartitionsByNames should return sorted partitions
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
kishen@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java
File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java:
http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@463
PS2, Line 463: (part) -> MetastoreShim.makePartName(partitionColNames, part.getValues())));
> Is there a way to do this only for the tests to avoid sorting potentially l
If this is done only for testing. We can just have a helper utility method, which will take two lists and ensure that all the members of a list is present in the other list. Tests can just use this utility method for assertion.
--
To view, visit http://gerrit.cloudera.org:8080/17812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256
Gerrit-Change-Number: 17812
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <ki...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sourabh Goyal <so...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 17:52:32 +0000
Gerrit-HasComments: Yes