Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command ...................................................................... Patch Set 16: (15 comments) http://gerrit.cloudera.org:8080/#/c/10744/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/16//COMMIT_MSG@40 PS16, Line 40: 4. the quiesce period (which is ideally set to the AC queueing : delay plus some additional leeway) expires : 5. once the daemon is drained (i.e. no fragments, no registered : queries), it shuts itself down. > 5 seems like also a form of quiescing (or continuation of the quiescing pro Yeah I went through a few iterations but I think it could be improved. Here are the changes I made: * quiesce period -> "startup grace period"/"grace period" depending on context * idle -> quiesced (for consistency - no need to use both idle and quiesced). http://gerrit.cloudera.org:8080/#/c/10744/16//COMMIT_MSG@45 PS16, Line 45: longer timeout > when does the clock start on this? after 4 or at the time of the request? Clarified. Checked other places if it was ambiguous and I think it's ok http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/scheduling/scheduler.cc@191 PS16, Line 191: is_quiescing > presumably this remains set after the "quiescing deadline" has passed, whic Hopefully the terminology changes elsewhere should make this clearer. http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.h@1196 PS16, Line 1196: > ... registered queries and ... ? Done http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10744/16/be/src/service/impala-server.cc@216 PS16, Line 216: started queries to complete executing before > I found this a bit confusing: isn't it more the period for queries to finis You're right. I reworked the text a little bit to be hopefully clearer. http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@513 PS16, Line 513: enum TAdminRequestType { > would be good to comment the struct Done http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@518 PS16, Line 518: the statement > what's "the statement"? Done http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@519 PS16, Line 519: If the port was specified, it is set in 'backend'. If : // it was not specified, it is 0 and the port configured for this Impala daemon is : // assumed. > do we need these multiple cases? is that to make it easier to test in the m Yeah mainly for the minicluster. I think it's possible to set up a "real" cluster with impala daemons on different ports too, although I don't know why anyone would do that. http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@528 PS16, Line 528: struct TAdminRequest { > comment the struct Done http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@531 PS16, Line 531: 2: optional TShutdownParams shutdown_params > presumably only one of these lists would be set, corresponding to the type Done http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift@857 PS16, Line 857: // Deadline for the shutdown. > what happens at the deadline? is that documented somewhere else? I updated the comment here. http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift@872 PS16, Line 872: registered > what does that mean? Registered in the "impala-server" sense for coordinato The first one. Updated the comment. Also renamed to 'client_requests_registered' which is more precisely the intent. http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/ImpalaInternalService.thrift@881 PS16, Line 881: 2: required TShutdownStatus shutdown_status > given the fields in status, does the caller make multiple calls to RemoteSh There's no method to poll the shutdown status right now. The returned status struct is used in a couple of places like the debug web UI to show the current status. It's also useful if there was a previous shutdown call with an earlier effective deadline. http://gerrit.cloudera.org:8080/#/c/10744/16/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/10744/16/fe/src/main/cup/sql-parser.cup@2509 PS16, Line 2509: COLON > do other systems use this syntax? how was it chosen (other than to avoid ke It was pretty arbitrary. I didn't find a good alternative. Most systems seem to add admin statements as top-level SQL statements, which runs into the problem of having to add a keyword for every new statement. Some systems (e.g. postgres) implement them as builtin functions that you invoke in a select query - https://www.postgresql.org/docs/9.4/static/functions-admin.html. This sort-of makes sense for a single-node database but feels wonky for a distributed database - it doesn't really make sense to run an administrative operation multiple times or on non-coordinators. Semantics are also kind of weird - if you have multiple admin functions in a select query, do we guarantee they run in a particular order? We could work around that by restricting the shapes of SELECT queries that could use admin functions but it seemed more complicated. http://gerrit.cloudera.org:8080/#/c/10744/16/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/16/fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java@65 PS16, Line 65: sb > does this have the leading colon? missing toSql() test case? Done - added the test cases. -- 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: 16 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: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 10 Sep 2018 17:18:30 +0000 Gerrit-HasComments: Yes
