Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/11337 )
Change subject: IMPALA-7335: Fix a race in HdfsScanNode ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc@398 PS1, Line 398: // If there was already an error, the main thread will do the cleanup : if (!status_.ok()) break; does that still work as expected now that ProcessSplit already set status_? http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc@401 PS1, Line 401: if (status.IsCancelled() && done_) { : // Scan node initiated scanner thread cancellation. No need to do anything. : break; : } : // Set status_ before calling SetDone() (which shuts down the RowBatchQueue), : // to ensure that GetNextInternal() notices the error status. : status_ = status; : SetDoneInternal(); : break; : } can this be removed now that it's handled in ProcessSplit()? Hmm, I guess we still need it for the case that StartNextScanRange() returned non-OK, but I'd make this more explicit so it's clearer how this all works. http://gerrit.cloudera.org:8080/#/c/11337/1/be/src/exec/hdfs-scan-node.cc@514 PS1, Line 514: // HdfsScanNodeBase::status_ variable and notify other scanner threads. It seems like the whole scanner thread protocol could really use a cleanup. But if you want to go with this kind of fix in the meantime, it'd be good to explicitly state in the comment what the dependency is here -- that status_ needs to be set before whatever it is. -- To view, visit http://gerrit.cloudera.org:8080/11337 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id470818846a5c69ad28a6ff695069702982aa793 Gerrit-Change-Number: 11337 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 27 Aug 2018 21:42:00 +0000 Gerrit-HasComments: Yes
