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 6: (3 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. The first non-ok status is set and all subsequent errors are ignored. those two sentences seem to contradict each other. If calling it repeatedly ignores subsequent calls then subsequent errors would also be ignored. 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 at line 519? If so, maybe make that more explicit to clarify the dependency. http://gerrit.cloudera.org:8080/#/c/11337/6/be/src/exec/hdfs-scan-node.cc@538 PS6, Line 538: // The scan node's execution completed, preserve the status_ currently set. does status_ still get set outside of SetDoneInternal()? If so, why? -- 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: 6 Gerrit-Owner: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhecht.apa...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 17 Sep 2018 19:01:12 +0000 Gerrit-HasComments: Yes
