Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16375 )
Change subject: IMPALA-10076: Reduce partition level update logs ...................................................................... Patch Set 4: (5 comments) Thank Vihang's review! Refactored the patch. Hope it's more readable now. 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? We need it. Otherwise, these is a compilation error: [ERROR] /home/quanlong/workspace/Impala/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:[249,10] no suitable method found for info(org.apache.impala.catalog.PartitionMetaSummary) method org.slf4j.Logger.info(java.lang.String) is not applicable (argument mismatch; org.apache.impala.catalog.PartitionMetaSummary cannot be converted to java.lang.String) method org.slf4j.Logger.info(java.lang.String,java.lang.Object...) is not applicable (argument mismatch; org.apache.impala.catalog.PartitionMetaSummary cannot be converted to java.lang.String) method org.slf4j.Logger.info(org.slf4j.Marker,java.lang.String,java.lang.Object...) is not applicable (argument mismatch; org.apache.impala.catalog.PartitionMetaSummary cannot be converted to org.slf4j.Marker) I think the reason is that org.slf4j.Logger#info() doesn't have an override method accepting an Object argument like we usually see in System.out.println(Object) which will calls toString() internally. So here we need to call toString() explicitly. 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: could be more than 100K, so we just keep > would be good to say "store the lexicographically smallest, second smallest Done http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@114 PS3, Line 114: if (delete) { > nit, personally I feel the code is more readable if avoid usage of this pat Done http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@120 PS3, Line 120: > I was bit confused by the meaning of smallest partName. Would be good to sa Done http://gerrit.cloudera.org:8080/#/c/16375/3/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@126 PS3, Line 126: // Init the lexico > nit, this line becomes confusing because of the post-increment operator abo Sure. Sorry to make you confused. I refactor this part to be more readable (but slightly inefficient). -- 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: 4 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: Mon, 14 Sep 2020 02:36:22 +0000 Gerrit-HasComments: Yes
