Sai Hemanth Gantasala has posted comments on this change. ( http://gerrit.cloudera.org:8080/21045 )
Change subject: IMPALA-12771: Impala catalogd events-skipped may mark the wrong number ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/21045/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21045/3//COMMIT_MSG@10 PS3, Line 10: event ,the metric will also be increased, besides for some other cases like alter nit: "event, the" http://gerrit.cloudera.org:8080/#/c/21045/3//COMMIT_MSG@11 PS3, Line 11: partition the event is skipped and the log is printed but the events-skipped metric Can you add more details in this commit message about what all events are addressed in this patch to correctly update the events-skipped metric? And can you also mention the test added in this patch? under the section "testing:" http://gerrit.cloudera.org:8080/#/c/21045/1/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/21045/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1055 PS1, Line 1055: } else if (numPartsRefreshed == -1) { > Hi, the reason that I change the catalogOpExecutor_.reloadPartitionsIfExist I think hdfsTable.reloadPartitionsFromNames() would return 0 only if the partitions returned from metastore are zero. (simultaneous drop in metastore while reloading partitions in impala). So I think it would make sense to me to return 0 and make condition as "else". http://gerrit.cloudera.org:8080/#/c/21045/3/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/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1089 PS3, Line 1089: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); Shouldn't this be: "metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(partitions.size());" http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1094 PS3, Line 1094: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); nit: same as L#1089 http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1098 PS3, Line 1098: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); nit: same as L#1089 http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1112 PS3, Line 1112: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); nit: ".inc(partitionNames.size())" http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1117 PS3, Line 1117: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); nit: same as L#1112 http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1121 PS3, Line 1121: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); nit: same as L#1121 http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1745 PS3, Line 1745: metrics_.getCounter(MetastoreEventsProcessor.EVENTS_SKIPPED_METRIC).inc(); Instead of incrementing the metric here, we can directly evaluate the boolean value in the reloadTableFromCatalog() method and then increment the metric in that method. So that we can avoid duplications in L#2326-2329, L#2538-2541, L#2654-2657, L#2780-2783, L#3005-3008, L#3138-3141. http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/21045/3/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@237 PS3, Line 237: // the catalogd. Can you update this description to also include Reload event and blacklisted metadata objects? Also, we increment this when event processor is not in active state(disabled). -- To view, visit http://gerrit.cloudera.org:8080/21045 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7aeb04e999b82187eb138c0b643ead259da22f1a Gerrit-Change-Number: 21045 Gerrit-PatchSet: 3 Gerrit-Owner: Anonymous Coward <cclive1...@gmail.com> Gerrit-Reviewer: Anonymous Coward <cclive1...@gmail.com> Gerrit-Reviewer: Anonymous Coward <k.venureddy2...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Sai Hemanth Gantasala <saihema...@cloudera.com> Gerrit-Comment-Date: Mon, 01 Apr 2024 23:36:59 +0000 Gerrit-HasComments: Yes
