Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8414 )
Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/8414/8/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/8414/8/be/src/runtime/io/request-context.h@171 PS8, Line 171: // boost doesn't let us dcheck that the reader lock is taken Outdated comment http://gerrit.cloudera.org:8080/#/c/8414/8/be/src/runtime/io/request-context.h@294 PS8, Line 294: InternalQueue<ScanRange> cached_ranges_; It seems these queues don't need the spinlocks in them. But we don't need to change them in this patch. http://gerrit.cloudera.org:8080/#/c/8414/8/be/src/runtime/io/request-context.h@366 PS8, Line 366: void DecrementRequestThread(const boost::unique_lock<boost::mutex>& context_lock, How about DecrementDiskThread? I don't see the term "request thread" elsewhere. http://gerrit.cloudera.org:8080/#/c/8414/8/be/src/runtime/io/request-context.h@378 PS8, Line 378: if (is_on_queue_.Load() == 0 && num_threads_in_op_.Load() == 0 && !done_) { I see the correctness here. It's pretty subtle. http://gerrit.cloudera.org:8080/#/c/8414/8/be/src/runtime/io/request-context.h@386 PS8, Line 386: /// If done is true, it means that this request must not be on this disk's queue Is this statement true? I think RequestContext::Cancel() can enqueue the context after it's set to done. -- To view, visit http://gerrit.cloudera.org:8080/8414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1 Gerrit-Change-Number: 8414 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 16 Nov 2017 22:45:52 +0000 Gerrit-HasComments: Yes
