Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22338 )
Change subject: [compaction] Release delta blocks memory early to avoid more consumption. ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/22338/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22338/3//COMMIT_MSG@7 PS3, Line 7: to avoid more consumption Given https://gerrit.cloudera.org/#/c/22079/ is already published and merged, could you share some stats on the amount of memory used by major delta compactions before and after this patch? http://gerrit.cloudera.org:8080/#/c/22338/1/src/kudu/tablet/delta_iterator_merger.cc File src/kudu/tablet/delta_iterator_merger.cc: http://gerrit.cloudera.org:8080/#/c/22338/1/src/kudu/tablet/delta_iterator_merger.cc@124 PS1, Line 124: const unique_ptr<DeltaIterator>& iter nit: this could be 'const auto& iter' http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/delta_store.h@366 PS3, Line 366: virtual Status DeinitDeltaBlocks() = 0; Please add a documentation blurb for this new method. In particular (as in opposite to the Init() method), it's necessary to document the side effects after calling this method, e.g. what methods from the interface of this class are still supposed to work as expected once this method is called? http://gerrit.cloudera.org:8080/#/c/22338/1/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/22338/1/src/kudu/tablet/delta_store.h@366 PS1, Line 366: DeinitDeltaBlocks Might 'FreeDeltaBlocks()' be a better name for this? http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/deltafile.h File src/kudu/tablet/deltafile.h: http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/deltafile.h@271 PS3, Line 271: delta_blocks_.clear(); How to make sure the cleared 'delta_blocks_' aren't mistakenly accessed elsewhere in the code as if they still were around with their original contents after calling this method? http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/deltamemstore.h File src/kudu/tablet/deltamemstore.h: http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/deltamemstore.h@241 PS3, Line 241: if it's necessary Is this still an open question, i.e. did you find it's necessary to do so or not? http://gerrit.cloudera.org:8080/#/c/22338/3/src/kudu/tablet/deltamemstore.h@243 PS3, Line 243: Status::OK() Is it safe to return Status::NotSupported() here instead given the implementation isn't present? -- To view, visit http://gerrit.cloudera.org:8080/22338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0c1eadcb8ad735fc92ac77f554934e606e4119ae Gerrit-Change-Number: 22338 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Sat, 01 Feb 2025 02:06:56 +0000 Gerrit-HasComments: Yes
