Henry Robinson has posted comments on this change. Change subject: IMPALA-4160: Remove Llama support. ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/4445/1//COMMIT_MSG Commit Message: Line 7: Remove Llama support. > You beat me to it in patch set #2 Done PS1, Line 31: * Remove RM flags (--enable_rm etc.) : * Remove RM query options : * Changes to RequestPoolService. > reference https://issues.cloudera.org/browse/IMPALA-4159 Done http://gerrit.cloudera.org:8080/#/c/4445/1/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: PS1, Line 69: RuntimeProfile* profile, int64_t byte_limit, : const std::string& label, MemTracker* parent) > 1 line? I will clang-format this patch separately. PS1, Line 87: MemTracker::MemTracker(UIntGauge* consumption_metric, : int64_t byte_limit, const string& label) > 1 line? See above. http://gerrit.cloudera.org:8080/#/c/4445/1/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: PS1, Line 174: // TODO: This may not be right if more than one tracker can actually change its : // RM reservation limit. > remove Done PS1, Line 179: // TODO: this doesn't roll it back completely since the max values for : // the updated trackers aren't decremented. The max values are only used : // for error reporting so this is probably okay. Rolling those back is : // pretty hard; we'd need something like 2PC. > I think this was talking about the RM case so this can be removed. Done PS1, Line 190: new limit=" << limit << " prev=" << limit; > new/prev isn't interesting anymore, this is enough: Done http://gerrit.cloudera.org:8080/#/c/4445/1/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: PS1, Line 112: per_host_mem = query_options_.rm_initial_mem; > I'd be fine with taking out this case and removing the query option- I don' I don't think we can or should remove the query option now, as that would break some clients I think. I'll leave a TODO to remove this logic if admission control depends on this. I could see someone using this option to force admission control behavior by capping the mem estimate. PS1, Line 120: FLAGS_rm_default_memory > same as above. Done PS1, Line 125: // TODO: Get this limit from Llama (Yarn sets it). > remove Done http://gerrit.cloudera.org:8080/#/c/4445/2/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: PS2, Line 88: int64_t GetClusterMemoryEstimate() const; > I think this can be removed Done http://gerrit.cloudera.org:8080/#/c/4445/2/fe/src/main/java/com/cloudera/impala/planner/Planner.java File fe/src/main/java/com/cloudera/impala/planner/Planner.java: PS2, Line 295: > We can remove the VCores stuff now, though some users may be relying on the Decided against this - users might rely on this for capacity estimations etc. MT might completely change the logic, in which case we can remove then. http://gerrit.cloudera.org:8080/#/c/4445/2/testdata/cluster/node_templates/cdh5/etc/hadoop/conf/yarn-site.xml.tmpl File testdata/cluster/node_templates/cdh5/etc/hadoop/conf/yarn-site.xml.tmpl: PS2, Line 35: <property> : <name>yarn.scheduler.fair.continuous-scheduling-enabled</name> : <value>true</value> : </property> : : <property> : <name>yarn.scheduler.fair.assignmultiple</name> : <value>true</value> : </property> > I think a lot of this is stuff that was needed for Llama. Can you leave a T Done. I think we are very close to being able to remove this file. -- To view, visit http://gerrit.cloudera.org:8080/4445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icfb14209e31f6608bb7b8a33789e00411a6447ef Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
