[Impala-ASF-CR] IMPALA-1760: Implement shutdown command

["Dan Hecht (Code Review)" <ge...@cloudera.org>]

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 <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 04 Sep 2018 22:13:36 +0000
Gerrit-HasComments: Yes