Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command ...................................................................... Patch Set 16: (15 comments) This generally looks good to me. Mostly minor comments or thoughts on terminology. 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 process), so the name quiesce deadline seemed confusing to me (i.e. i was expecting that to be the deadline described in 6 - i.e. quiescing of this daemon hasn't completed). Maybe call the thing in #4 the admission grace period or query startup grace period or something? if others find the current terminology self explanatory, okay to ignore. 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? 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, which is more reason the name of that deadline can be confusing. 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 ... ? 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 finish starting, i.e. period before we'll use metrics to determine whether the node has actually quiesced? 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 http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@518 PS16, Line 518: the statement what's "the statement"? 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 mini-cluster, or is there a different reason for that? http://gerrit.cloudera.org:8080/#/c/10744/16/common/thrift/Frontend.thrift@528 PS16, Line 528: struct TAdminRequest { comment the struct 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 tag? i.e. this is a union? 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? 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 coordinators, or something else? 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 RemoteShutdown() to get updated status, or something? 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 keywords)? 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? -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 04 Sep 2018 22:13:36 +0000 Gerrit-HasComments: Yes
