[Impala-ASF-CR] IMPALA-7738: Implement timeouts for HDFS open calls

["Joe McDonnell (Code Review)" <ge...@cloudera.org>]

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 <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 01 Dec 2018 01:21:34 +0000
Gerrit-HasComments: Yes