Dan Hecht has posted comments on this change. Change subject: IMPALA-3857: KuduScanNode race on returning "optional" threads ......................................................................
Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/3637/2/be/src/exec/kudu-scan-node.cc File be/src/exec/kudu-scan-node.cc: PS2, Line 451: num_active_scanners_ > 1 isn't this redundant with the: optional_threads > num_reserved_optional_threads_ check inside of optional_exceeded() and the call ReserveOptionalTokens(1)? rather than encoding that we need at least 1 thread in both places, it might be slightly better to put the ReleaseThreadToken(false) call under the lock and get rid of this check and the num_active_scanners_ decrement here. It's still pretty gross due to the messed up ResourcePool interface, but maybe slightly less so. PS2, Line 456: TODO: Refactor so the 'check if done' state change is atomic. yes, we really should do that. I'm not sure I understand your concern about abstraction on the kudu-scan-node class given this would be a private helper method, so not really part of the abstraction. (It's just C++ forcing you to put private stuff in header files but that's always annoying). -- 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: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
