Joe McDonnell 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) Added custom cluster test, rebased, and fixed some minor issues. 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 Added a test that STOPs the NameNode and verifies that a query times out. I looked into having a debug action cause a sleep, but plumbing that through the layers of threads is pretty annoying. I think over the longer term, DiskIo threads should know a lot more about what query they are executing on behalf of, but for now, that isn't there. 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. Done 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? Done 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/? Done 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? Yes, at the INFO level, this logs twice. Once from status.cc and once here. status.cc does not log at the WARNING level, so this will only show up once there. -- 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: Sat, 01 Dec 2018 01:21:34 +0000 Gerrit-HasComments: Yes
