Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/10744 )
Change subject: IMPALA-1760: Implement shutdown command ...................................................................... Patch Set 8: (13 comments) still need to review tests 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.... 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 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 operation. http://gerrit.cloudera.org:8080/#/c/10744/8/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/10744/8/be/src/service/impala-server.h@385 PS8, Line 385: an nit: a 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 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 http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@361 PS6, Line 361: ft nit: a http://gerrit.cloudera.org:8080/#/c/10744/6/be/src/service/impala-server.h@362 PS6, Line 362: ly. nit:before 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 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 change during the lifecycle of the impalad 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 difference in "quiesce" and "deadline" here http://gerrit.cloudera.org:8080/#/c/10744/6/common/thrift/ImpalaInternalService.thrift@867 PS6, Line 867: er nit: deadline 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 -- 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: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 03 Jul 2018 01:36:55 +0000 Gerrit-HasComments: Yes
