Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command ...................................................................... Patch Set 7: (19 comments) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@46 PS7, Line 46: does extra word? http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG@56 PS7, Line 56: Limitations: I think it would be good to mention that the shutdown cannot be aborted. Even better would be to make it abortable, e.g. add another cancel_shutdown() command. That would also help validate the design of the admin fn utility code. Another minor limitation seems to be that we cannot quiesce a daemon without shutting it down. Should we expose the two-stage nature of the process through explicit commands? That could help with debugging, i.e. we could isolate a misbehaving machine without killing it. This could be a follow up change, too. http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.h@a451 PS7, Line 451: What happened to LOAD DATA? http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc@612 PS7, Line 612: avoid nit: avoids http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/client-request-state.cc@626 PS7, Line 626: RETURN_IF_ERROR(client_status); Should we retry if we get a stale client (similar to here: https://github.com/apache/impala/blob/4ce2af9ff2340bba1bfb40a1ed3b5b39d6011f87/be/src/runtime/coordinator-backend-state.cc#L363) ? http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/impala-server.cc@216 PS7, Line 216: drain I'd rephrase it as "wait for queries" since they might not even have arrived yet. "drain" reads as if they're already running. Also, mention the unit of time in the help string, too? http://gerrit.cloudera.org:8080/#/c/10744/7/be/src/service/impala-server.cc@2328 PS7, Line 2328: while (deadline_val == 0 || deadline_val > absolute_deadline_ms) { maybe name these "curr_deadline_val" and "new_deadline_val"? or something similar? http://gerrit.cloudera.org:8080/#/c/10744/7/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/10744/7/common/thrift/ImpalaInternalService.thrift@881 PS7, Line 881: struct TShutdownStatus { Why not fold this into the Result RPC below? http://gerrit.cloudera.org:8080/#/c/10744/7/common/thrift/Types.thrift File common/thrift/Types.thrift: http://gerrit.cloudera.org:8080/#/c/10744/7/common/thrift/Types.thrift@104 PS7, Line 104: comment comment? command? maybe rename to ADMIN_CMD then? http://gerrit.cloudera.org:8080/#/c/10744/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/10744/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@95 PS7, Line 95: * Supports optionally specify the backend and the deadline, either shutdown(), nit: grammar http://gerrit.cloudera.org:8080/#/c/10744/7/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@128 PS7, Line 128: private Pair<Expr, Expr> getShutdownArgs() throws AnalysisException { I think this might be more readable with a loop, parsing each expr and setting the class members as a side effect. http://gerrit.cloudera.org:8080/#/c/10744/7/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/10744/7/fe/src/main/java/org/apache/impala/analysis/Expr.java@1546 PS7, Line 1546: * Analyzes and evaluates expression to a non-negative integral value, returned as a long. nit: long line http://gerrit.cloudera.org:8080/#/c/10744/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/10744/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@3807 PS7, Line 3807: ParsesOk(":foobar()"); Can you add some with arguments? http://gerrit.cloudera.org:8080/#/c/10744/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@3831 PS7, Line 3831: ParserError(": shutdown() :other()"); How about ": shutdown('hostA'); : shutdown('hostB'); ? http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py@108 PS7, Line 108: """Test that idle impalads shut down in a timely manner after the quiesce period nit: This comment seems to conflict with the comment in L113. http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py@115 PS7, Line 115: e6c00ca5cd67b567eb96c6ecfb26f05 nit: would this fit in a single line with a shorter hostname? http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py@238 PS7, Line 238: conf_overlay={"NUM_SCANNER_THREADS": "1"}, close_session=False) Could you pass this as a default QO to the impalad options? http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py@271 PS7, Line 271: self.execute_statement("select 1", None, TCLIService.TStatusCode.ERROR_STATUS, : SHUTDOWN_ERROR_PREFIX) This doesn't seem to depend on the local function above and I couldn't see why it's here. Move above? http://gerrit.cloudera.org:8080/#/c/10744/7/tests/custom_cluster/test_restart_services.py@278 PS7, Line 278: check_hs2_shutdown_error(self.hs2_client.GetTypeInfo( : TCLIService.TGetTypeInfoReq(self.session_handle))) duplicate of above? -- To view, visit http://gerrit.cloudera.org:8080/10744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0 Gerrit-Change-Number: 10744 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 03 Jul 2018 03:27:24 +0000 Gerrit-HasComments: Yes
