Quanlong Huang 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 8: (7 comments) > I had a quick glance at it (except for the test), I've got some suggestions. Thanks for the comments! Addressed them. 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: required TEventProcessorCmdParams params > When can this be omitted? Should this also be required? Yeah, this should 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: functions a > Nit: "functions are" now that we've added another function. Done 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. Done http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@101 PS7, Line 101: super.analyze(analyzer); > This comment is no longer true. Done 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: if (!startEventProcessorHelper(params, ep, resp, info)) { > Could extract the "start" case into a function. Done http://gerrit.cloudera.org:8080/#/c/22250/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3790 PS7, Line 3790: if (params.getEvent_id() < -1) { > What happens if params.getEvent_id() is zero or some negative number (other Good point! We can throw an IllegalArgumentException for it. Maybe zero is ok if we want EventProcessor to reprocess all the events. But values smaller than -1 currently makes no sense. We can reject them. 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: setEventProcessorSta > Is there a reason why this is not called "setEventProcessorStatus()"? It's the original name. Forgot to remane this.. -- 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: 8 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: Wed, 22 Jan 2025 09:25:29 +0000 Gerrit-HasComments: Yes
