Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22079 )
Change subject: [compaction] Add memory tracking for better OOM issues triaging ...................................................................... Patch Set 10: (6 comments) http://gerrit.cloudera.org:8080/#/c/22079/9/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: http://gerrit.cloudera.org:8080/#/c/22079/9/src/kudu/tablet/delta_compaction.cc@132 PS9, Line 132: op may not complete > This patch does not take any action to avoid OOM condition or suggest opera Thanks for the clarification. http://gerrit.cloudera.org:8080/#/c/22079/9/src/kudu/tablet/delta_compaction.cc@193 PS9, Line 193: // Update memory tracker with memory allocated for mutations. > Yes, the idea is to protect against negative values. Now that I think about OK, them maybe then get rid of those 'if(...)' in the code and just call 'UpdateMemTracker()' with corresponding deltas? http://gerrit.cloudera.org:8080/#/c/22079/9/src/kudu/tablet/delta_compaction.cc@271 PS9, Line 271: for (const DeltaKeyAndUpdate& key_and_update : out) { > Memory arena usage is pretty low as compared to other places objects like d OK, but then why to account for Arena's memory usage above at lines 237 and 262 in PS9? If you do account for it there, then account for it elsewhere: otherwise it looks like a bug. What do you think? http://gerrit.cloudera.org:8080/#/c/22079/10/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: http://gerrit.cloudera.org:8080/#/c/22079/10/src/kudu/tablet/delta_compaction.cc@130 PS10, Line 130: if (process_memory::UnderMemoryPressure(&capacity_pct)) { OK, since this looks rather a warning about possible OOM condition per clarification provided in comments for PS9, I'd think the memory pressure condition isn't quite relevant here, but we should rather compare CurrentConsumption() with HardLimit() as is? Also, since at this point we know that major delta and rowset merge compactions aren't actually honoring the memory limit, probably --memory_limit_warn_threshold_percentage isn't relevant here. Also, the memory accounting with MemoryTrackers is approximate, not exact. With that, I'd think of introducing one more flag (and maybe corresponding method as well) in ProcessMemory to log warnings about going over the hard memory limit with big enough margin (say, 25% or around by default), so it's clear we are witnessing something else than rounding errors and fuzziness of memory accounting with MemoryTrackers. What do you think? http://gerrit.cloudera.org:8080/#/c/22079/10/src/kudu/tablet/delta_compaction.cc@131 PS10, Line 131: msg Why to spend CPU cycles and memory to build 'msg' here if it's only needed within the 'if()' clause below? http://gerrit.cloudera.org:8080/#/c/22079/10/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/22079/10/src/kudu/tablet/delta_store.h@482 PS10, Line 482: auto delta Would 'const auto& delta' be a better option here to avoid extra copying? -- To view, visit http://gerrit.cloudera.org:8080/22079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2037582d433730884212e83359bb75bad0d5394 Gerrit-Change-Number: 22079 Gerrit-PatchSet: 10 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Thu, 23 Jan 2025 03:21:13 +0000 Gerrit-HasComments: Yes
