impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joe McDonnell (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5750: Catch exceptions from boost thread creation
Date Thu, 31 Aug 2017 23:57:53 GMT
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5750: Catch exceptions from boost thread creation
......................................................................


Patch Set 7:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

PS7, Line 371: COUNTER_ADD(num_scanner_threads_started_counter_, 1);
> The problem with moving this here is when the node is under heavy stress, t
Yes, this could be off. I suppose there is no way around it.


http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/exec/kudu-scan-node.cc
File be/src/exec/kudu-scan-node.cc:

PS7, Line 171: ++num_active_scanners_;
> This can cause quite a few races. It can race with L220, L244 and L245.
num_active_scanners_ is protected by lock_. We get lock_ at the top of this loop and the other
locations get lock_.


http://gerrit.cloudera.org:8080/#/c/7730/6/be/src/runtime/query-exec-mgr.cc
File be/src/runtime/query-exec-mgr.cc:

PS6, Line 59:   status = Thread::Create("query-exec-mgr",
            :       Substitute("start-query-finstances-$0", PrintId(query_id)),
            :           &QueryExecMgr::StartQueryHelper, this, qs, &t, true);
> Done
In testing, I found that this also needs qs->ReleaseInitialReservationRefcount(), which
was taken in Init(). Added.


http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

PS7, Line 335:     // Fragment instance successfully started
             :     // update fis_map_
             :     fis_map_.emplace(fis->instance_id(), fis);
             :     // update fragment_map_
             :     vector<FragmentInstanceState*>& fis_list = fragment_map_[instance_ctx.fragment_idx];
             :     fis_list.push_back(fis);
> Is it safe to update the map with the Fragment instance state after already
My thought process is something like this:
fis is a local variable. Both fis_map_ and fragment_map_ are private variables on QueryState.
None of these variables are referenced outside this QueryState except through APIs protected
by the promise. It looks like the fragment instance doesn't care about using the functions
that would access these variables.

Worth considering if there is any way around it, but I think it is safe.


http://gerrit.cloudera.org:8080/#/c/7730/7/be/src/util/thread.cc
File be/src/util/thread.cc:

PS7, Line 303: rand()
> Not to be pedantic, but a rand() without seeding the PRNG first, will cause
I was thinking it might make sense for us to do a single srand() at startup. I will look into
the appropriate place to do this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I15a2f278dc71892b7fec09593f81b1a57ab725c0
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message