Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10245 )
Change subject: IMPALA-6953: clean up DiskIoMgr ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/disk-io-mgr-internal.h File be/src/runtime/io/disk-io-mgr-internal.h: http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/disk-io-mgr-internal.h@73 PS7, Line 73: void DiskThreadLoop(DiskIoMgr* io_mgr); The main worker loop is now in DiskQueue rather than DiskIoMgr, but the logic didn't change. http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/disk-io-mgr-internal.h@89 PS7, Line 89: void ShutDown(); The ShutDown() logic did change to use a local shut_down_ variable per-DiskQueue rather than a DiskIoMgr member variable. http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/disk-io-mgr.cc@493 PS7, Line 493: RequestRange* DiskQueue::GetNextRequestRange(RequestContext** request_context) { This function was broken up into multiple functions, but the logic should be equivalent. http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.h@496 PS7, Line 496: }; > is there a way to coerce this diff to show more what has actually changed? The non-whitespace diff for PerDiskState is below. The only substantive difference is that IncrementDiskThreadAfterDequeue() is now wrapped by RequestContext @@ -1,6 +1,6 @@ - /// Struct containing state per disk. See comments in the disk read loop on how - /// they are used. - class PerDiskState { +/// Struct containing state per disk. See comments in the disk read loop on how +/// they are used. +class RequestContext::PerDiskState { public: bool done() const { return done_; } void set_done(bool b) { done_ = b; } @@ -38,10 +38,7 @@ void ScheduleContext(const boost::unique_lock<boost::mutex>& context_lock, RequestContext* context, int disk_id); - /// Called when dequeueing this RequestContext from the disk queue to increment the - /// count of disk threads with a reference to this context. These threads do not hold - /// any locks while reading from HDFS, so we need to prevent the RequestContext from - /// being destroyed underneath them. + /// See RequestContext::IncrementDiskThreadAfterDequeue() comment for usage. /// /// The caller does not need to hold 'lock_', so this can execute concurrently with /// itself and DecrementDiskThread(). @@ -152,4 +149,4 @@ /// GetNextRequestRange() and GetNextUnstartedRange() may result in only reads being /// processed) InternalQueue<WriteRange> unstarted_write_ranges_; - }; +}; > }; http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.h@498 PS7, Line 498: inline void RequestContext::IncrementDiskThreadAfterDequeue(int disk_id) { These functions got pulled out of the class body, but the logic shouldn't have changed. http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.cc File be/src/runtime/io/request-context.cc: http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.cc@a27 PS7, Line 27: This code got moved to scan-range.cc but the logic didn't change. http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.cc@a57 PS7, Line 57: Moved to BufferDescriptor::Free() http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-context.cc@216 PS7, Line 216: RequestRange* RequestContext::GetNextRequestRange(int disk_id) { This was broken out of DiskIoMgr::GetNextRequestRange(). The logic should be preserved. http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/10245/7/be/src/runtime/io/request-ranges.h@291 PS7, Line 291: /// Reads from the DN cache. On success, sets cached_buffer_ to the DN buffer These functions were moved from private: to protected: but not changed. -- To view, visit http://gerrit.cloudera.org:8080/10245 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a6e393f8c01d10143cbac91108af37f6498c1b1 Gerrit-Change-Number: 10245 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 22 May 2018 00:29:37 +0000 Gerrit-HasComments: Yes
