Lars Volker has posted comments on this change. Change subject: IMPALA-5147: Add the ability to exclude hosts from query execution ......................................................................
Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/scheduling/scheduler-test-util.cc File be/src/scheduling/scheduler-test-util.cc: Line 61: int Cluster::AddHost(bool has_backend, bool has_datanode, bool is_executor) { Can you DCHECK(!is_executor || has_backend)? http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/scheduling/scheduler-test.cc File be/src/scheduling/scheduler-test.cc: Line 56: /// ranges. nit: single line Line 59: cluster.AddHost(true, true, false); This looks like it can get hard to parse rather quickly - I think even having two boolean flags is a suboptimal. Can we consider something more clear? For example chaining function calls: cluster.AddHost().SetBackend().SetDatanode(); I'm open to anything really, if you can't think of anything better, I'm also good with leaving it like it is. http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: Line 32: #include "rapidjson/rapidjson.h" still needed? Line 44: using namespace rapidjson; still needed? Line 204: // If this is not an executor don't add it to the new backend config. nit: punctuation Line 205: if (be_desc.is_executor) { Do you need to check __is_set? http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 2000: vector<string> modes; Building this with a stringstream may be easier and would also not need another boost dependency. http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 272: rapidjson::Document* document); you could forward declare the class and move the include to the .cc file. http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/util/webserver.cc File be/src/util/webserver.cc: Line 199: if (env->impala_server()->IsCoordinator()) modes.push_back("Coordinator"); Same here, maybe use stringstream? http://gerrit.cloudera.org:8080/#/c/6628/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: PS1, Line 216: missing word? Line 254: else: elif? http://gerrit.cloudera.org:8080/#/c/6628/1/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 65: // True if this is a coordinator node What does it mean if these are unset? http://gerrit.cloudera.org:8080/#/c/6628/1/tests/custom_cluster/test_coordinators.py File tests/custom_cluster/test_coordinators.py: Line 94: operators are executed on 'expected_num_of_executors' nodes.""" nit: indendation -- To view, visit http://gerrit.cloudera.org:8080/6628 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5d2ff7f341c9d2b0649e4d14561077e166ad7c4d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-HasComments: Yes
