Amogh Margoor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17413 )
Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr ...................................................................... Patch Set 5: (25 comments) http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@250 PS4, Line 250: class ScanRange : public RequestRan > We need to specify what fields it protects in ScanRange. Or, we should stil Removed this class. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@252 PS4, Line 252: > nit. needs Code deleted. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@514 PS4, Line 514: > +1 on "ensuring no new locks are taken in ScanBufferManager methods. This is done now. No locks acquired in ScanBufferManager. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h File be/src/runtime/io/scan-buffer-manager.h: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@36 PS4, Line 36: ange using this buffer manager. > Actually there isn't much info at 'external_buffer_tag_'. Could you add com Have updated the documentation. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@40 PS4, Line 40: /// This buffer is external to this buffer manager and is not managed by it > nit. missing '.'. Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@47 PS4, Line 47: > nit. nullptr. Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@52 PS4, Line 52: > nit. should be 'scan_range_'. Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@53 PS4, Line 53: > nit. this scan range (i.e. scan_range_). Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@62 PS4, Line 62: > nit. scan_range_->len(). Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@106 PS4, Line 106: scan_range_loc > nit. PopFirstReadyBuffer() describes the semantics better. Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@109 PS4, Line 109: /// Clean > nit. should be inline. Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@131 PS4, Line 131: /// ScanRange using thi > Instead of the friend declaration, can we add public functions that ScanRan Have removed friend from both the classes. Neither ScanRange can access private methods/members of ScanBufferManager nor the other way is possible now. This also prevents ScanBufferManager taking ScanRange's lock_. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@136 PS4, Line 136: void SetCachedBuffer() { : external_buffer_tag_ = ExternalBuffe > I think having this pointer is a bit confusing, probably it'd be a bit clea This code is deleted now as discussed. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@169 PS4, Line 169: iomgr_buffer_cu > nit: maybe name it BufferTag, since the buffer is managed by this new class That's the name from the existing code and it still is a tag used for external buffers. External buffers are tagged as CLIENT_BUFFER and CACHED_BUFFER, whereas managed buffers NO_BUFFER (to depict that it is not an external buffer). http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@177 PS4, Line 177: > nit: add +2 indentation Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc File be/src/runtime/io/scan-buffer-manager.cc: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@28 PS4, Line 28: mplementation of the buffer managem > This probably should be moved inside the cstr body, after DCHECK(). Code is deleted now. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@85 PS4, Line 85: nused_iomgr_buffer_bytes_ -= result->buffer_le > please update comment Done. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@135 PS4, Line 135: ::CleanUpBu > nit. can we replace goto with the actual code below? done. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@162 PS4, Line 162: Buff > this->scan_range? done. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@165 PS4, Line 165: return false; : } : *buffer = move(ready_buffers_.front()); : ready_buffers_.pop_front(); : // If eosr is seen, then unused buffer should be empty. : DCHECK(!(*buffer)->eosr() || unused_iomgr_buffers_.empty()) << DebugString(); : return true; : } : : bool > It looks like this block of code is scan_range_ specific and probably shoul it has been moved to new method as suggested. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@199 PS4, Line 199: << " eosr_queued: " > Should return false if the buffer is empty as DCHECK() only works in debug Thats a precondition i.e., callers of PopReadyBuffer (like ScanRange::GetNext()) needed to ensure that ready_buffer_ is not empty, hence only DCHECK is used. Additionally, return value 'false' meant eosr has not been hit. It is not semantically same as ready_buffer_ being empty - for instance ready_buffer_ can be empty after eosr has been hit. But this API returning eosr status didn't make a lot of sense - it should just do one thing which is to pop an element from ready_buffer_. Hence I have changed the API and precondition is not required now. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc File be/src/runtime/io/scan-range.cc: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@88 PS4, Line 88: the first ready buff > nit. This data member does not exist anymore. changed it. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@427 PS4, Line 427: ScanRange::Reset(hdfsFS fs, const char* file, int64_t len, int64_t offset, : int disk_id, bool expected_local, int64_t mtime, const B > nit. Call buffer_manager_->is_client_buffer() instead? This is an assignment not a check. I guess you meant to add a Setter which is done. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@432 PS4, Line 432: HECK(!read_in_flight_); : DCHECK(file != nullptr); > Same comment as above. This is an assignment not a check. I guess you meant to add a Setter which is done. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@553 PS4, Line 553: { : unique_lock<mutex> lock(lock_); > nit. May create a set method buffer_manager_ and call it SetCachedBuffer(tr yep tags should be hidden. Added functions. -- To view, visit http://gerrit.cloudera.org:8080/17413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 5 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 26 May 2021 11:14:33 +0000 Gerrit-HasComments: Yes
