Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11874 )
Change subject: IMPALA-7738: Implement timeouts for HDFS open calls ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/11874/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11874/4//COMMIT_MSG@17 PS4, Line 17: 5 minutes How is this determined ? In this patch, only hdfsOpen() has been ported over to have a timeout. Do we anticipate more HDFS operations to be ported over in the future ? If so, will they need a different timeout periods ? http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/handle-cache.inline.h File be/src/runtime/io/handle-cache.inline.h: http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/handle-cache.inline.h@33 PS4, Line 33: /* TODO: check return code */ nit: I believe we tend to use // for inline comments http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.h File be/src/runtime/io/hdfs-monitored-ops.h: http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.h@18 PS4, Line 18: IMPALA_RUNTIME_IO_HDFS_SHIM_H nit: should match file name http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.h@30 PS4, Line 30: // nit: /// http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.cc File be/src/runtime/io/hdfs-monitored-ops.cc: http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.cc@32 PS4, Line 32: HDFS operation should wait before timing out and failing Please see comments below in HdfsMonitor::OpenFileHandle(). It appears that a call to Open() can potentially block for longer than this timeout period. http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.cc@55 PS4, Line 55: virtual Status ExecuteImpl(); Why not just make it a pure virtual function ? and we don't need line 70 - line 73 below? http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.cc@66 PS4, Line 66: // Set when ExecuteImpl() completes Set to return status of ExecuteImpl() upon completion. http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/runtime/io/hdfs-monitored-ops.cc@139 PS4, Line 139: FLAGS_hdfs_operation_timeout_sec * MILLIS_PER_SEC); Both the Offer() and Wait() calls can block for almost FLAGS_hdfs_operation_timeout_sec seconds, right ? So, this function can in theory block for 2 * FLAGS_hdfs_operation_timeout_sec seconds, right ? So, should we be more clear about exactly what a HDFS operation in the definition (hdfs_operation_timeout_sec) means ? http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/util/thread-pool.h File be/src/util/thread-pool.h: http://gerrit.cloudera.org:8080/#/c/11874/4/be/src/util/thread-pool.h@116 PS4, Line 116: bool Offer(V&& work, int64_t timeout_millis) { May warrant a test in thread-pool-test.cc -- 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: 4 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 08 Nov 2018 20:19:18 +0000 Gerrit-HasComments: Yes
