Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16375 )
Change subject: IMPALA-10076: Reduce partition level update logs ...................................................................... Patch Set 2: (4 comments) Thank you for working on the observability. I had a few minor comments but otherwise LGTM. Vihang, do you plan to review or should I try to bring it to +2 myself? http://gerrit.cloudera.org:8080/#/c/16375/2/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/16375/2/be/src/catalog/catalog-server.cc@729 PS2, Line 729: return -1; There's an overflow here, since the return value is an unsigned int - it'll wrap around to the maximum value. This would probably cause a UBSAN error. I'd suggest making the return value an int32 or just an int. http://gerrit.cloudera.org:8080/#/c/16375/2/be/src/catalog/catalog-server.cc@737 PS2, Line 737: // Skip logging partition items since FE will log their summary. Maybe leave the JIRA here for context? http://gerrit.cloudera.org:8080/#/c/16375/2/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/16375/2/be/src/service/fe-support.cc@483 PS2, Line 483: uint32_t int or int32_t to avoid the overflow http://gerrit.cloudera.org:8080/#/c/16375/2/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/2/fe/src/main/java/org/apache/impala/catalog/PartitionMetaSummary.java@125 PS2, Line 125: } else if (partName.compareTo(partNames[0]) < 0) { This all feels a little complex, but I think it makes sense and I don't think it's worth overengineering since it's so encapsulated. -- 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: 2 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: Wed, 02 Sep 2020 00:57:33 +0000 Gerrit-HasComments: Yes
