Add support 'keepSegmentGranularity' for compactionTask #6095
Conversation
| return Collections.singletonList( | ||
| new IndexIngestionSpec( | ||
| dataSchema, | ||
| new IndexIOConfig( |
There was a problem hiding this comment.
maybe worth pulling out a method or lambda fn maybe IndexIOConfig makeConfig(dataSchema, interval) since constructor statements are pretty big and largely the same?
There was a problem hiding this comment.
Sounds good. Fixed.
| log.warn("Interval[%s] has no segments, nothing to do.", interval); | ||
| return TaskStatus.failure(getId()); | ||
| } else { | ||
| final String json = jsonMapper.writerWithDefaultPrettyPrinter().writeValueAsString(indexTaskSpec); | ||
| log.info("Generated compaction task details: " + json); | ||
| log.info("Generated [%d] compaction task specs: ", indexTaskSpecs.size()); |
There was a problem hiding this comment.
I think you can get rid of the trailing colon, since nothing is really coming after it.
| return indexTaskSpec.run(toolbox); | ||
| final TaskStatus eachResult = eachSpec.run(toolbox); | ||
| if (!eachResult.isSuccess()) { | ||
| log.warn("Running indexSpec failed. Trying to the next indexSpec."); |
There was a problem hiding this comment.
What if it throws an exception?
Also, "Trying to the next indexSpec" isn't grammatical, it should be "Trying the next indexSpec".
There was a problem hiding this comment.
Good point. Fixed.
| @@ -245,12 +264,12 @@ public TaskStatus run(final TaskToolbox toolbox) throws Exception | |||
| * | |||
| * @return null if input segments don't exist. Otherwise, a generated ingestionSpec. | |||
There was a problem hiding this comment.
This javadoc should be updated too.
| final GranularitySpec granularitySpec = new ArbitraryGranularitySpec( | ||
| new NoneGranularity(), | ||
| GranularityType.NONE.getDefaultGranularity(), |
There was a problem hiding this comment.
Granularities.NONE is a simpler way of writing this.
| @@ -150,8 +150,11 @@ private CoordinatorStats doRun( | |||
|
|
|||
| if (segmentsToCompact.size() > 1) { | |||
| final DataSourceCompactionConfig config = compactionConfigs.get(dataSourceName); | |||
| // Currently set keepSegmentGranularity to false because it breaks the algorithm of CompactionSegmentIterator to | |||
There was a problem hiding this comment.
Do you plan to address this in a future patch?
There was a problem hiding this comment.
Yes. I'll do in a follow-up PR.
This PR introduces a new configuration,
keepSegmentGranularity, to compactionTask. Once this is set to true, the compactionTask respects the existing segment boundaries and doesn't compact segments across those segment boundaries.From the implementation side, the compactionTask generates multiple indexTaskSpecs per segment interval and runs them sequentially. This should be fine because there's no (or very little) performance penalty compared to running a single indexTaskSpec.
Another characteristics of this is, the compactionTask is always finished as
SUCCEEDEDno matter how many indexTaskSpecs succeed. This means, the application (the coordinator in automatic compaction) is responsible for checking which segments the compactionTask failed to compact and rerunning another compactionTask.This option is enabled by default in compactionTask, but automatic compaction currently disables this because it breaks the algorithm of
NewestSegmentFirstIterator. I'll fix and enable in the follow-up pr.