Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11874 )
Change subject: IMPALA-7738: Implement timeouts for HDFS open calls ...................................................................... Patch Set 9: (5 comments) Looks generally good to me. Some minor comments. http://gerrit.cloudera.org:8080/#/c/11874/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11874/9//COMMIT_MSG@28 PS9, Line 28: 2. Core tests I'd like to see a test that calls "kill -STOP" to suspend the namenode and trigger a timeout. Alternatively, we might be able to inject a sleep using https://github.com/btraceio/btrace for the namenode, or something like that. Or, conceptually, we could just add a sleep() right before the real hdfsOpen() call that's controlled by one of our debug options... Anyway, it seems like we should make sure that the error case of the hdfsOpen() timing out is covered. http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/runtime/io/hdfs-monitored-ops.h File be/src/runtime/io/hdfs-monitored-ops.h: http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/runtime/io/hdfs-monitored-ops.h@50 PS9, Line 50: Status OpenHdfsFileWTimeout(const hdfsFS& fs, const std::string* fname, int flags, I see "With" spelled out in other cases. Not a big deal. http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/runtime/io/hdfs-monitored-ops.cc File be/src/runtime/io/hdfs-monitored-ops.cc: http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/runtime/io/hdfs-monitored-ops.cc@48 PS9, Line 48: : fs_(fs), fname_(std::string(*fname)), flags_(flags), blocksize_(blocksize) {} Should this just be fname_(*fname) to invoke the copy constructor? http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/util/thread-pool.h File be/src/util/thread-pool.h: http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/util/thread-pool.h@272 PS9, Line 272: /// Otherwise, it returns the status returned by ExecuteImpl(). s/ExecuteImpl/Execute/? http://gerrit.cloudera.org:8080/#/c/11874/9/be/src/util/thread-pool.h@337 PS9, Line 337: LOG(WARNING) << timeout_status.GetDetail(); In practice, does this end up logging twice? -- To view, visit http://gerrit.cloudera.org:8080/11874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia14403ca5f3f19c6d5f61b9ab2306b0ad3267454 Gerrit-Change-Number: 11874 Gerrit-PatchSet: 9 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 29 Nov 2018 19:13:36 +0000 Gerrit-HasComments: Yes
