impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Ribeiro Alves (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3857: KuduScanNode race on returning "optional" threads
Date Thu, 14 Jul 2016 01:57:01 GMT
David Ribeiro Alves has posted comments on this change.

Change subject: IMPALA-3857: KuduScanNode race on returning "optional" threads
......................................................................


Patch Set 1:

(2 comments)

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

Line 421:   bool optional_threads_exceeded = false;
> I'll take a look, though I'm not too worried about it because it repros con
yeah only if it's easy (not sure how involved is to emulate runtime_state_->resource_pool()->optional_exceeded())
but would be good to have some coverage that doesn't require running tpch under load.


Line 480:   if (!optional_threads_exceeded) --num_active_scanners_;
> I don't think it's so easy. The issue is that if we don't check the state a
oh, I see the race you are talking about. I'm regretting not having changed the goto's from
the original implementation and making this more vanilla. Can and move mentioning the race
to where you decrement num_active_scanners_ in the block above and leave a TODO to refactor
this logic? this is totally something that future me would forget


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I22adf2109b43b1b37d9a597de85e063431dff155
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dralves@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message