Matthew Jacobs 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: Yeah that's a good idea. 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 Sure. I think abstraction was the wrong way to phrase it, rather just that my first attempt to refactor this didn't result in obviously better code. There are a few different cases to consider for exiting/cleaning up: 1) There are no more key ranges. 2) Observe that the scan node is already done_ 2) An error is observed, i.e. some status is not OK 4) The number of optional threads is exceeded Each case is handled in one or several different places and with slightly differently behaviors. It's a bit ugly but I'll find something better. I was just being a bit lazy since it was taking some time and it wasn't looking much cleaner to me, but I think with your point about the redundancy above makes it clear this is hacky. -- 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
