Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command ...................................................................... Patch Set 8: (30 comments) http://gerrit.cloudera.org:8080/#/c/10744/7//COMMIT_MSG Commit Message: 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. Ev Agree these are useful to note but this commit message is already out of control. Might be easier to track in JIRA. I wonder if we should rename is_shutting_down to is_quiesced or something like that in the statestore topic if we're going to generalise it later. http://gerrit.cloudera.org:8080/#/c/10744/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10744/8//COMMIT_MSG@39 PS8, Line 39: which is > how about: which ideally is set to the AC.... Done 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? There wasn't an implementation of this function, just cleaned up the unused declaration. http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/client-request-state.cc@616 PS6, Line 616: > nit:avoids Done 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 Done http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-hs2-server.cc@381 PS6, Line 381: HS2_RETURN_IF_ERROR(return_val, CheckNotShuttingDown(), SQLSTATE_GENERAL_ERROR); > we can probably remove the check here since its not starting any new operat That brings up a good point - I was a bit uncertain about what the best thing to do with requests like this. On one hand we could just continue to serve them and then if the client continues to send them at some point the connection will drop and the client will get a socket error or something similar. I thought that it might be better to fail with a clear error earlier so it's more obvious why the operation failed. It may be that there's no perfect solution without better integration with load balancer and clients. http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@354 PS6, Line 354: otherwi > nit: shutdown_status Done http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@358 PS6, Line 358: > dont mention private vars in public method comments. also at L367 Done http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@361 PS6, Line 361: ft > nit: a Done http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@362 PS6, Line 362: ly. > nit:before Done http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.cc@212 PS6, Line 212: > nit: long line Done 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 arrive Done 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 s Done http://gerrit.cloudera.org:8080/#/c/10744/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/10744/8/be/src/service/impala-server.cc@1772 PS8, Line 1772: && is_shutting_down == it->second.is_shutting_down) { > maybe mention briefly that is_shutting_down is the only part that can chang Done http://gerrit.cloudera.org:8080/#/c/10744/6/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/10744/6/common/thrift/ImpalaInternalService.thrift@865 PS6, Line 865: 5: optional TBloomFilter bloom_filter > not sure if mentioned elsewhere in the code, but maybe we can explain the d This probably isn't the best place to document since it's just a status message. I think I probably need to write a class-level comment in ImpalaServer to explain this holistically. http://gerrit.cloudera.org:8080/#/c/10744/6/common/thrift/ImpalaInternalService.thrift@867 PS6, Line 867: er > nit: deadline not sure how i ended up with that word 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? The motivation was to allow using TShutdownStatus in both the local and remote shutdown paths - see ClientRequestState::ExecShutdownRequest(). On the local path the function returns a Status, so returning a TStatus inside a struct would be confusing. http://gerrit.cloudera.org:8080/#/c/10744/8/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/10744/8/common/thrift/ImpalaInternalService.thrift@931 PS8, Line 931: // Called to initiate shutdown of this backend. : TRemoteShutdownResult RemoteShutdown(1:TRemoteShutdownParams params); > Yes, I think if you cherry-pick that patch mentioned above, you should be a Ok, maybe we'll wait to see who wins the race. I don't mind porting, would be good to learn a little bit about the KRPC infrastructure. 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? Reworded to "function" 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 Done 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 sett I had an earlier iteration of the code that did something like that and I felt it was harder to follow when the logic to set members was interleaved with this logic. 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 Done http://gerrit.cloudera.org:8080/#/c/10744/8/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/8/fe/src/main/java/org/apache/impala/analysis/Expr.java@1546 PS8, Line 1546: ong > nit:long line Done 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? Done 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'); ? Done 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. Fixed in next PS 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? Probably, the idea here was that an sha1 hash was extremely unlikely to be a valid hostname and making it shorter increases the odds of a collision. Not sure if there's a better way to pick a hostname that's guaranteed to be invalid. 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? Done 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 Done 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? Done -- 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: 8 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: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 05 Jul 2018 05:43:37 +0000 Gerrit-HasComments: Yes
