Zoltan Borok-Nagy 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 4: (1 comment) 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@514 PS4, Line 514: lock_store_ > Actually the logic in ScanBufferManager and ScanRange are still coupled and EnqueueReadyBuffer() and AddUnusedBuffer() acquire the lock at the beginning, so it wouldn't make harm to pre-acquire the locks. But CleanUpBuffers() also acquires the lock, and it is invoked by AllocateBuffersForRange() at the error path. Taking a look again AddUnusedBuffer() is also invoked by AllocateBuffersForRange() in the middle of the method. One way to resolve this is to put AllocateBuffersForRange() back to ScanRange and acquiring the lock there, so AddUnusedBuffer() and CleanUpBuffers() and all other member functions in ScanBufferManager can expect a pre-acquired lock. I think I like this idea as it'll probably make the reasoning clearer about locking. -- 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: 4 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 19 May 2021 16:48:48 +0000 Gerrit-HasComments: Yes
