added Pause option into minor compactions #5289
Conversation
| @@ -392,7 +393,13 @@ DataFileValue minorCompact(InMemoryMap memTable, ReferencedTabletFile tmpDatafil | |||
| Span span = TraceUtil.startSpan(this.getClass(), "minorCompact::write"); | |||
| try (Scope scope = span.makeCurrent()) { | |||
| count = memTable.getNumEntries(); | |||
|
|
|||
| if (count > pauseLimit && pauseLimit > 0) { | |||
There was a problem hiding this comment.
I don't think we want to pause minor compactions when the number of entries in tablet exceeds the file count pause property. We only want to pause minor compactions when the number of files in the tablet exceeds the file count pause property. I think that the change in this file can be removed.
| currentFileCount); // Add pause check here | ||
| } catch (AcceptableThriftTableOperationException e) { | ||
| log.warn("Minor compaction paused due to file count for tablet {}", tablet.extent); | ||
| return; |
There was a problem hiding this comment.
I don't think that returning here will cause a minor compaction to be paused. I think the calling code will look at this as being completed successfully, right? I'm not sure this is the right solution. @keith-turner - what was your vision of this change from #5151 ?
There was a problem hiding this comment.
I would add the check here. If this code returns null then the minor compaction will never start. That code can consider the reason when making this decision. We could return null in the case where mincReason == SYSTEM and the tablets file count would exceed the limit. Probably do not want to prevent minor compactions for other reasons like log recovery or tablet close (tserver may have too many tablets and need to shed them). Returning null at that point in the code avoids putting anything on the thread pool and reserving things in the tablet memory (as long as prepareForMinC() is not called that is) that need to be properly unreserved in the case where it can not minor compact, so it seems like a good place to intercept.
There are some race conditions w/ the check at that location, the tablet could surge over the limit but would eventually pause in a stable system(one that is not migrating tablets). The bulk import code has the same behavior, it can surge over the limit but will eventually pause. Avoiding the race condition completely would require doing the minor compaction and checking the file count in the conditional mutation that adds the file to the tablet. This would be much more complex and not worth the effort IMO. Doing the check before queing the compaction is good even if there is race condition because the tablet will eventually pause and when it does it will keep pausing until the file count goes down providing the needed back pressure to keep the tablet healthy.
Added a check in MinorCompactionTask to ensure that the current number of files being set to compact are below the Pause property threshold.