Quanlong Huang 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 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/21045/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21045/8//COMMIT_MSG@9 PS8, Line 9: The description of events-skipped metric is wrong. Some cases in Add partition : event ,the metric will also be increased, besides for some other cases like alter : partition the event is skipped and the log is printed but the events-skipped metric : is not increased. Please format this to be 72 characters width. Add space after comma and remove space before comma. Add periods when neededd. Please also explain the solution. http://gerrit.cloudera.org:8080/#/c/21045/8/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/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1764 PS8, Line 1764: // both old table not removed and new table not add, that means we skip the : // rename event process if one is true and the other is false, that means we : // need to use the function(removeTableIfNotAddedLater/addTableIfNotRemovedLater) : // to do the real event process. nit: might be more clear if reword this to "Only bump the skipped metric if the old table is not removed and the new table is not added. Not doing this in other cases since we need to either remove the old table or add the new table, which is processing the event." http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1811 PS8, Line 1811: if (isOlderEvent(null)) { Should we also bump the skipped metric in this case and L1819 for isSelfEvent()? http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@1842 PS8, Line 1842: if (notSkipped) { Shouldn't this be !notSkipped? http://gerrit.cloudera.org:8080/#/c/21045/8/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/8/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@241 PS8, Line 241: . B nit: remove period "." and use lowercase "b". This should be the same sentence. http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4879 PS8, Line 4879: return -1; Could you add a comment about this? Why should we use -1 here? http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@5018 PS8, Line 5018: , nit: add a space after comma "," http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1995 PS8, Line 1995: // is IncompleteTable and addPartition event may also skip. Do you mean sometimes we could see more events being skipped, e.g. 3 or 4? http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2033 PS8, Line 2033: assertTrue("Event fetch duration should be greater than zero", : response.getEvents_fetch_duration_mean() > 0); : assertTrue("Event process duration should be greater than zero", : response.getEvents_process_duration_mean() > 0); These two are unrelative to the skipped metric. I think we can remove them. http://gerrit.cloudera.org:8080/#/c/21045/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@2043 PS8, Line 2043: // alter event process will skip; nit: remove semicolon ";" and reword to "ALTER events will be skipped." -- 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: 8 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 11 Jun 2024 07:28:17 +0000 Gerrit-HasComments: Yes
