impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3902: Scheduler improvements for running multiple fragment instances on a single backend
Date Mon, 12 Sep 2016 22:28:22 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3902: Scheduler improvements for running multiple fragment instances
on a single backend
......................................................................


Patch Set 4:

(27 comments)

http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

PS4, Line 218:   const TNetworkAddress local_addr_;
> Can you use ExecEnv::backend_address()? Or find some way to avoid the dupli
Done


PS4, Line 297: Wait
> nit: Wait()
Done


PS4, Line 388: fragment_instance_state_idxs
> Is this referencing the "fragment id (TPlanFragment.id)"? If so, please say
it's not, it's an index into fragment_instance_states_. i thought that would be clear from
the name.

added comment.


http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/query-schedule.cc
File be/src/scheduling/query-schedule.cc:

PS4, Line 121:   std::sort(fragments.begin(), fragments.end(),
             :       [](const TPlanFragment* a, const TPlanFragment* b) { return a->id
< b->id; });
> shouldn't these come in an expected order?
this adds an extra level of robustness, otherwise i have to make this ordering guarantee part
of the interface.

i don't expect this to have any performance impact.


PS4, Line 346: NULL
> nullptr
Done


http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/query-schedule.h
File be/src/scheduling/query-schedule.h:

Line 97:   std::map<PlanNodeId, int> per_exch_num_senders;
> any benefit from using map over unordered_map here?
it makes it easier to assign to thrift structs, and this isn't performance-critical.


PS4, Line 173: NULL
> nullptr
Done


Line 174:   const TPlanFragment* GetCoordFragment() const; 
> nit: trailing whitespace
Done


Line 213:   MtFragmentExecParams* GetFragmentExecParams(FragmentId id) {
> If you need non-const access, isn't it more idiomatic to return a non-const
we typically return pointers for mutable structures (and const refs for immutable structures).


PS4, Line 219: NULL
> nullptr
Done


Line 307: 
> comment, e.g. what state in MtFragmentExecParams gets initialized here vs. 
Done


http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/simple-scheduler.cc
File be/src/scheduling/simple-scheduler.cc:

PS4, Line 384: auto
> auto only for iterators
this is a map iterator


PS4, Line 414: depth-first traversal
> isn't this an implementation detail of the other MtComputeFragmentExecParam
Done


PS4, Line 425: f
> can you change f indexes here to match the code? it's explained as if it st
moved comment


PS4, Line 433:       const TDataStreamSink& sink = src_fragment.output_sink.stream_sink;
             :       DCHECK(
             :           sink.output_partition.type == TPartitionType::UNPARTITIONED
             :             || sink.output_partition.type == TPartitionType::HASH_PARTITIONED
             :             || sink.output_partition.type == TPartitionType::RANDOM);
> move this closer to where it's used on l453
Done


PS4, Line 470: MtComputeFragmentExecParams
> Can we be sure that recursion depth will not be a problem here?
yes


Line 524:     // try to load-balance scan ranges by assigning just beyond the
> nit: wrap at 90 characters
Done


PS4, Line 541:       while (params_idx < params_list.size()
             :           && total_assigned_bytes < threshold_total_bytes) {
> It's not obvious to me why this will always assign all params. If the last 
i'm not sure what to add.

for the last fragment, threshold_total_bytes == total_size, so this won't get exceeded. if
total_assigned_bytes == total_size, by definition there can't be any params left.

let me know how i can clarify.


Line 695:     for (int j = 0; j < params.hosts.size(); ++j, ++num_fragment_instances) {
> you could call reserve() before this line
i'll leave the old code as-is.


PS4, Line 933:     if (!FLAGS_disable_admission_control) {
             :       RETURN_IF_ERROR(admission_controller_->AdmitQuery(schedule));
             :     }
             :     if (!FLAGS_enable_rm) return Status::OK();
             : 
             :     string user = GetEffectiveUser(schedule->request().query_ctx.session);
             :     if (user.empty()) user = "default";
             :     schedule->PrepareReservationRequest(resolved_pool, user);
             :     const TResourceBrokerReservationRequest& reservation_request =
             :         schedule->reservation_request();
             :     if (!reservation_request.resources.empty()) {
             :       Status status = resource_broker_->Reserve(
             :           reservation_request, schedule->reservation());
             :       if (!status.ok()) {
             :         // Warn about missing table and/or column stats if necessary.
             :         const TQueryCtx& query_ctx = schedule->request().query_ctx;
             :         if(!query_ctx.__isset.parent_query_id &&
             :             query_ctx.__isset.tables_missing_stats &&
             :             !query_ctx.tables_missing_stats.empty()) {
             :           status.AddDetail(GetTablesMissingStatsWarning(query_ctx.tables_missing_stats));
             :         }
             :         return status;
             :       }
             :       RETURN_IF_ERROR(schedule->ValidateReservation());
             :       AddToActiveResourceMaps(*schedule->reservation(), coord);
             :     }
> while the llama related code will be removed soon, I think you should still
Done


Line 1104:       it = backend_map_.insert(make_pair(ip, BackendList())).first;
> BackendConfig has been moved to an own file in asf/master, and this code ha
i'm sure it will, but i'm holding off on rebases for now.


http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/scheduling/simple-scheduler.h
File be/src/scheduling/simple-scheduler.h:

PS4, Line 489:  
> nit: whitespace, wrap at 90 characters
Done


PS4, Line 492: mt_fragment_exec_params_
> Is there a way around referring to a private member of the schedule here?
this class deals with populating private members of that class, so no.


PS4, Line 499: MtComputeFragmentExecParams
> The separation between what structures are set/updated in QuerySchedule and
QuerySchedule is really just a helper class which functions as a container for scheduling
data. some of that data comes from the initial TExecRequest, some is created by the scheduling
logic. i moved all static setup functions as well as purely mechanical functions into QuerySchedule
(such as figuring out the number of remote fragment instances), but i think the actual scheduling
logic should stay in the scheduler.


PS4, Line 502: depth-first traversal
> Is this implementation-detail relevant for the caller?
it's needed for understanding the correctness - the bottom-up process relies on having scheduling
data for the children before scheduling the parent fragment.


PS4, Line 504: MtComputeFragmentExecParams
> It'd be nice to have a different and more specific name, e.g. MtCreateFInst
i would argue that the current name is more specific, because it does more than just the FInstanceExecParams.
i could call this MtComputeMtFragmentExecParams, but didn't think that was necessary. also,
keep in mind that the 'mt' prefixes are only temporary.


http://gerrit.cloudera.org:8080/#/c/4054/4/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

PS4, Line 439:   // TODO-MT: if exec_request_.query_options.mt_dop isn't set, set it to the
pool
             :   // default, if any
> This should already happen in impala-{hs2,beeswax}-server when the query co
removed


-- 
To view, visit http://gerrit.cloudera.org:8080/4054
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I240445199e22f009f4e72fdb8754eb8d77e3d680
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message