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 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_ > Now this lock protects the members of the scan range, and the members of th Actually the logic in ScanBufferManager and ScanRange are still coupled and both modify each others member variables currently. Additionally there are parts in ScanBufferManager which needs ScanRange lock to be freed before scheduling scan range due to existing ordering between locks of ScanRange and RequestContext. So with this coupling it would be cleaner to have only one lock (which we can keep in ScanRange as suggested instead of moving to new class). However, we can make semantics cleaner by ensuring no new locks are taken in ScanBufferManager methods. ScanRange's lock is pre-acquired (if needed) before invoking the buffer manager's methods. All existing method follows that except EnqueueReadyBuffer and AddUnusedBuffer which I can refactor and ensure it follows that. Would that make things better ? -- 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 <amarg...@gmail.com> Gerrit-Reviewer: Amogh Margoor <amarg...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 19 May 2021 15:46:21 +0000 Gerrit-HasComments: Yes
