Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-5147: Add the ability to exclude hosts from query execution ......................................................................
Patch Set 1: (15 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)? Done 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 Done Line 59: cluster.AddHost(true, true, false); > This looks like it can get hard to parse rather quickly - I think even havi Agree, this isn't pretty. Using a builder may not work well with AddHost(). The latter is not merely a factory for hosts, it also populates several Cluster fields, like list of backends, etc. I'll leave a TODO for now and give it some thought later. 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? Done Line 44: using namespace rapidjson; > still needed? Done Line 204: // If this is not an executor don't add it to the new backend config. > nit: punctuation You want me to remove the punctuation? Line 205: if (be_desc.is_executor) { > Do you need to check __is_set? Made the fields required, so I guess it's not needed now. Line 207: current_membership_.insert(make_pair(item.key, be_desc)); > i thought the plan was to continue to have all nodes of the cluster still b I am a little confused with the last part of your comment: "That way, e.g. clients can still use the membership topic to discover coordinators." I couldn't find how that works exactly, can you plz elaborate or give some pointers? 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 ano Done 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. Done 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? Done http://gerrit.cloudera.org:8080/#/c/6628/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: PS1, Line 216: > missing word? Done Line 254: else: > elif? Done 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? I added those as optional because they aren't always set (e.g. during scheduler tests). I think it may be better to make them required. What do you think? 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 Done -- 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: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-HasComments: Yes
