Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19246 )
Change subject: WIP [compact] Increase chances of compaction for large number deltas ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/19246/1//COMMIT_MSG Commit Message: PS1: > It would be great to conduct at least a manual test to see how this change I have done manual testing with upserts that generated many deltas and I could see delta compactions being given priority frequently as compared to codebase without this change. In this change, I have kept threshold of deltas size as 1GB but I guess we can increase that further to avoid a case where compaction is being triggered more than desired times. I will take a look at HeavyUpdateCompactionITest to see if it is straightforward enough to include in this change else can be added in different change later. http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/delta_tracker.cc@513 PS1, Line 513: undo_redo->EstimateSize(); > Should we somehow account for compression here, if present? At least, havi I guess you are talking about data at rest is in compressed format and can be considered more in size (maybe double) when being processed ? So, something like this maybe: --- if (compressed) tmp_bytes += (2 * undo_redo->EstimateSize()); else tmp_bytes += undo_redo->EstimateSize(); --- I took inspiration from EstimateBytesInPotentiallyAncientUndoDeltas where UndoDeltaBlockGCOp also relies on the size returned from EstimateSize. Didn't notice any additional handling for compression. Btw, where is the information about whether the file is compressed or not, stored? http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/mock-rowsets.h File src/kudu/tablet/mock-rowsets.h: http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/mock-rowsets.h@162 PS1, Line 162: EstimateBytesInDeltas > Does it make sense to override this for MockDiskRowSet() with and add a tes Are you pointing to TestYcsbCompaction test? I am not clear on what we would verify in the new test to be added. Can you please elaborate more. http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/tablet.cc@2469 PS1, Line 2469: double workload_score = 0; > Does it make sense to expose a run-time flag to tweak this parameter? If you are talking about workload_score, I think it may make more sense to make max_limit_deltas_size run-time flag. That way, we can enforce the higher score by decreasing max_limit_deltas_size further. http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/tablet.cc@2470 PS1, Line 2470: // Need to fine tune limit > nit: usually, adding TODO(git_username) helps to track such places in futur Done http://gerrit.cloudera.org:8080/#/c/19246/1/src/kudu/tablet/tablet.cc@2482 PS1, Line 2482: workload_score = std::min(FLAGS_workload_score_upper_bound, : tablet_bytes / max_limit_deltas_size); > I guess we need to allow this to climb higher at least to get a bit higher That's a good point. I almost forgot about it even after pointed this out earlier. While I agree with the last approach of normalising the metrics for flush (maybe some other op as well) and ensure 1 as maximum. That would change the current logic of flush perf improvement and open up possibility of potential issue due to the impact of code change. In the interest of keeping the change limited to compaction of deltas, increasing the score to more than 1 would be of less impact. Something like this: +++ DEFINE_int32(delta_compaction_factor, 10, "Delta compaction op improvement factor"); -- double ratio = tablet_bytes / max_limit_deltas_size; if (ratio > 1) { workload_score = delta_compaction_factor * ratio; } else { workload_score = ratio; } +++ -- To view, visit http://gerrit.cloudera.org:8080/19246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2199ae3b777e75b15b60d8ad818cc6adc4f5fa3b Gerrit-Change-Number: 19246 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 15 Nov 2022 14:56:06 +0000 Gerrit-HasComments: Yes
