Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16375 )
Change subject: IMPALA-10076: Reduce partition level update logs ...................................................................... Patch Set 3: Code-Review+1 (5 comments) Thanks for this patch. Will definitely help with the log readability. The patch looks good to me. I mostly have some non-blocking suggestions below. http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@249 PS3, Line 249: toString() Isn't toString() redundant here? http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java File fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java: http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@55 PS3, Line 55: the smallest, second smallest and largest would be good to say "store the lexicographically smallest, second smallest and the largest partition name". http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@114 PS3, Line 114: numParts = numDeletedParts_++; nit, personally I feel the code is more readable if avoid usage of this pattern because of the way post-increment works. It is more clear to me if we do: numDeletedParts++; numParts = numDeletedParts; http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@120 PS3, Line 120: the current smallest one I was bit confused by the meaning of smallest partName. Would be good to say lexicographically smallest one. http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@126 PS3, Line 126: if (numParts == 2) nit, this line becomes confusing because of the post-increment operator above. The value of numDeletedParts_ is 2 but numParts will be 2 and hence the index of the array seems off by 1 unless you carefully read the code. May be add more comments here. -- To view, visit http://gerrit.cloudera.org:8080/16375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic48946b2f8b0be1e73988092d03a004836f1b368 Gerrit-Change-Number: 16375 Gerrit-PatchSet: 3 Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Fri, 11 Sep 2020 17:30:30 +0000 Gerrit-HasComments: Yes
