Henry Robinson has posted comments on this change.

Change subject: IMPALA-4160: Remove Llama support.

Patch Set 1:


Commit Message:

Line 7: Remove Llama support.
> You beat me to it in patch set #2

PS1, Line 31: * Remove RM flags (--enable_rm etc.)
            : * Remove RM query options
            : * Changes to RequestPoolService.
> reference https://issues.cloudera.org/browse/IMPALA-4159

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.

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

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.

PS1, Line 190: new limit=" << limit << " prev=" << limit;
> new/prev isn't interesting anymore, this is enough:

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.

PS1, Line 125:   // TODO: Get this limit from Llama (Yarn sets it).
> remove

File be/src/scheduling/query-schedule.h:

PS2, Line 88:   int64_t GetClusterMemoryEstimate() const;
> I think this can be removed

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.

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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to