Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22250 )
Change subject: IMPALA-12785: Add commands to control event-processor status ...................................................................... Patch Set 7: (7 comments) I had a quick glance at it (except for the test), I've got some suggestions. http://gerrit.cloudera.org:8080/#/c/22250/7/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/22250/7/common/thrift/CatalogService.thrift@765 PS7, Line 765: 3: optional TEventProcessorCmdParams params When can this be omitted? Should this also be required? http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java File fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java: http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@40 PS7, Line 40: function is Nit: "functions are" now that we've added another function. http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@91 PS7, Line 91: result.event_processor_cmd_params = new TEventProcessorCmdParams(action_); Nit: could add DCHECK(false) in an ELSE branch. http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@101 PS7, Line 101: // Only shutdown is supported. This comment is no longer true. http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3768 PS7, Line 3768: } else if (cmdType == MetastoreEventsProcessor.EventProcessorCmdType.START) { Could extract the "start" case into a function. http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3790 PS7, Line 3790: } else if (ep.getStatus() != EventProcessorStatus.ACTIVE) { What happens if params.getEvent_id() is zero or some negative number (other than -1), e.g. -2? Should we handle it? Or add a precondition check? http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java File fe/src/main/java/org/apache/impala/service/JniCatalog.java: http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/service/JniCatalog.java@531 PS7, Line 531: updateEventProcessor Is there a reason why this is not called "setEventProcessorStatus()"? -- To view, visit http://gerrit.cloudera.org:8080/22250 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a19f67264cfe06a1819a22c0c4f0cf174c9b958 Gerrit-Change-Number: 22250 Gerrit-PatchSet: 7 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[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, 21 Jan 2025 17:07:28 +0000 Gerrit-HasComments: Yes
