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: (8 comments) Did an initial round. Overall looks good, will do another rounds next week. 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: /// Lock protecting fields below. We need to specify what fields it protects in ScanRange. Or, we should still keep the 'lock_' there. Since there's only one lock here we probably don't need this class. Maybe we could just add a public Lock() method to ScanRange. 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 the ScanRangeBuffer as well, right? Probably it'd be clearer if we had a two locks, one in scan range, one in scan range buffer, and define a clear ordering for how they should be acquired when we need to lock both. 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: See external_buffer_tag_ for details. Actually there isn't much info at 'external_buffer_tag_'. Could you add comment that describes the semantics for each value? http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@131 PS4, Line 131: friend class ScanRange; Instead of the friend declaration, can we add public functions that ScanRange could use? http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@136 PS4, Line 136: /// Scan range lock store which are needed for accessing buffer. : ScanRangeLockStore* const lock_store_; I think having this pointer is a bit confusing, probably it'd be a bit cleaner to use the lock via 'scan_range_'. I think we should either have 2 separate locks for ScanRange and ScanRangeBuffer, or only keep the lock in ScanRange and remove this pointer. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@177 PS4, Line 177: st nit: add +2 indentation 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@85 PS4, Line 85: Implementation of the ScanRange functionality. please update comment http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@162 PS4, Line 162: this this->scan_range? -- 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: 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: Fri, 14 May 2021 16:05:07 +0000 Gerrit-HasComments: Yes
