Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/11337 )
Change subject: IMPALA-7335: Fix a race in HdfsScanNode ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/11337/5/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/11337/5/be/src/exec/hdfs-scan-node.cc@a341 PS5, Line 341: we should probably retain this dcheck if its still true. http://gerrit.cloudera.org:8080/#/c/11337/5/be/src/exec/hdfs-scan-node.cc@a409 PS5, Line 409: : : : : : : is it possible to have done_==true && status_.ok() && !status.ok() && !status.IsCancelled() If yes what is expected to happen before and after your change? http://gerrit.cloudera.org:8080/#/c/11337/5/be/src/exec/hdfs-scan-node.cc@340 PS5, Line 340: Upa nit: extra word http://gerrit.cloudera.org:8080/#/c/11337/5/be/src/exec/hdfs-scan-node.cc@511 PS5, Line 511: if (UNLIKELY(status_.ok())) SetDoneInternal(status); if this is not the first non-ok status then we expect done_ to be set to true, right? In that case you dont need this if-check and can only call SetDoneInternal() since it already takes care of cases where done_ is set and !status_.ok() http://gerrit.cloudera.org:8080/#/c/11337/5/be/src/exec/hdfs-scan-node.cc@534 PS5, Line 534: // The scan node completed without any error, hence the final status_ must by OK. is this a requirement for SetDone()? If yes, maybe put a dcheck here and mention that in the method comments -- 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: 5 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: Wed, 12 Sep 2018 22:47:57 +0000 Gerrit-HasComments: Yes
