Pooja Nilangekar has posted comments on this change. ( http://gerrit.cloudera.org:8080/11337 )
Change subject: IMPALA-7335: Fix a race in HdfsScanNode ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.h@181 PS6, Line 181: Calling it repeatedly ignores subsequent : /// calls. > those two sentences seem to contradict each other. If calling it repeatedly Done http://gerrit.cloudera.org:8080/#/c/11337/4/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/4/be/src/exec/hdfs-scan-node.cc@527 PS4, Line 527: DoneInternal(const St > yes you are right, if we get a cancelled status and done is not set then we Done http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc@512 PS6, Line 512: marking a range as complete > I don't think it's obvious where that happens. Is that in the Close() call Done http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc@538 PS6, Line 538: unique_lock<mutex> l(lock_); > does status_ still get set outside of SetDoneInternal()? If so, why? No, status_ is only set in SetDoneInternal(). http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc@538 PS6, Line 538: unique_lock<mutex> l(lock_); > if this is only called when the execution is complete, worth adding that to Done. However completion could be due to reading all the scan ranges, reaching the limit specified in the query or if the fragment is closed by the coordinator. So I added a comment stating " This function shouldn't be called if the scan node hits an error." Or should I rephrase it? -- 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: 7 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, 17 Sep 2018 23:34:06 +0000 Gerrit-HasComments: Yes
