cecemei commented on code in PR #18968:
URL: https://github.com/apache/druid/pull/18968#discussion_r2850138200
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionCandidateSearchPolicy.java:
##########
@@ -48,74 +45,11 @@ public interface CompactionCandidateSearchPolicy
int compareCandidates(CompactionCandidate candidateA, CompactionCandidate
candidateB);
/**
- * Checks if the given {@link CompactionCandidate} is eligible for compaction
- * in the current iteration. A policy may implement this method to skip
- * compacting intervals or segments that do not fulfil some required
criteria.
+ * Creates a {@link CompactionCandidate} after applying policy-specific
checks to the proposed compaction candidate.
*
- * @return {@link Eligibility#OK} only if eligible.
- */
- Eligibility checkEligibilityForCompaction(
- CompactionCandidate candidate,
- CompactionTaskStatus latestTaskStatus
- );
-
- /**
- * Describes the eligibility of an interval for compaction.
+ * @param candidate the proposed compaction
+ * @param eligibility initial eligibility from compaction config checks
+ * @return final compaction candidate
*/
- class Eligibility
- {
- public static final Eligibility OK = new Eligibility(true, null);
-
- private final boolean eligible;
- private final String reason;
-
- private Eligibility(boolean eligible, String reason)
- {
- this.eligible = eligible;
- this.reason = reason;
- }
-
- public boolean isEligible()
- {
- return eligible;
- }
-
- public String getReason()
- {
- return reason;
- }
-
- public static Eligibility fail(String messageFormat, Object... args)
- {
- return new Eligibility(false, StringUtils.format(messageFormat, args));
- }
-
- @Override
- public boolean equals(Object object)
- {
- if (this == object) {
- return true;
- }
- if (object == null || getClass() != object.getClass()) {
- return false;
- }
- Eligibility that = (Eligibility) object;
- return eligible == that.eligible && Objects.equals(reason, that.reason);
- }
-
- @Override
- public int hashCode()
- {
- return Objects.hash(eligible, reason);
- }
-
- @Override
- public String toString()
- {
- return "Eligibility{" +
- "eligible=" + eligible +
- ", reason='" + reason + '\'' +
- '}';
- }
- }
+ CompactionCandidate createCandidate(CompactionCandidate.ProposedCompaction
candidate, CompactionStatus eligibility);
Review Comment:
gotcha - can just return `CompactionMode` here and let callsite handle
creation of candidate.
##########
server/src/test/java/org/apache/druid/server/coordinator/duty/CompactSegmentsTest.java:
##########
@@ -1545,17 +1550,20 @@ private void verifySnapshot(
{
Map<String, AutoCompactionSnapshot> autoCompactionSnapshots =
compactSegments.getAutoCompactionSnapshot();
AutoCompactionSnapshot snapshot =
autoCompactionSnapshots.get(dataSourceName);
- Assert.assertEquals(dataSourceName, snapshot.getDataSource());
- Assert.assertEquals(scheduleStatus, snapshot.getScheduleStatus());
- Assert.assertEquals(expectedByteCountAwaitingCompaction,
snapshot.getBytesAwaitingCompaction());
- Assert.assertEquals(expectedByteCountCompressed,
snapshot.getBytesCompacted());
- Assert.assertEquals(expectedByteCountSkipped, snapshot.getBytesSkipped());
- Assert.assertEquals(expectedIntervalCountAwaitingCompaction,
snapshot.getIntervalCountAwaitingCompaction());
- Assert.assertEquals(expectedIntervalCountCompressed,
snapshot.getIntervalCountCompacted());
- Assert.assertEquals(expectedIntervalCountSkipped,
snapshot.getIntervalCountSkipped());
- Assert.assertEquals(expectedSegmentCountAwaitingCompaction,
snapshot.getSegmentCountAwaitingCompaction());
- Assert.assertEquals(expectedSegmentCountCompressed,
snapshot.getSegmentCountCompacted());
- Assert.assertEquals(expectedSegmentCountSkipped,
snapshot.getSegmentCountSkipped());
+ Assert.assertEquals(new AutoCompactionSnapshot(
Review Comment:
i find it captures all the diff in the snapshot better, e.x. one less
segment in skipped, one more segment in compressed, thus motivates me to make
this change.
##########
indexing-service/src/main/java/org/apache/druid/indexing/compact/CompactionJobQueue.java:
##########
@@ -282,18 +281,17 @@ private boolean startJobIfPendingAndReady(
}
// Check if the job is already running, completed or skipped
- final CompactionStatus compactionStatus = getCurrentStatusForJob(job,
policy);
- switch (compactionStatus.getState()) {
- case RUNNING:
+ final CompactionCandidate.TaskState candidateState =
getCurrentTaskStateForJob(job);
Review Comment:
I've considered use `org.apache.druid.indexer.TaskState`(the one used in
`CompactionTaskStatus` and not quite like it, because it doesn't have READY
state (only running/failed/success), we could 1. use a null value for ready
state, but then the simulator uses a map of TaskState; 2. or add a new READY/
NOT_RUN state, but then the existing `isRunnable` and `isComplete` method in
`org.apache.druid.indexer.TaskState` is somewhat confusing, which we might need
to update it if add a new enum value. Given that this is just an enum, i
figured it might be ok to use a new class.
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionCandidate.java:
##########
@@ -37,76 +39,200 @@
*/
public class CompactionCandidate
{
- private final List<DataSegment> segments;
- private final Interval umbrellaInterval;
- private final Interval compactionInterval;
- private final String dataSource;
- private final long totalBytes;
- private final int numIntervals;
-
- private final CompactionStatus currentStatus;
-
- public static CompactionCandidate from(
- List<DataSegment> segments,
- @Nullable Granularity targetSegmentGranularity
- )
+ /**
+ * Non-empty list of segments of a datasource being proposed for compaction.
+ * A proposed compaction typically contains all the segments of a single
time chunk.
+ */
+ public static class ProposedCompaction
Review Comment:
The motivation of this new `ProposedCompaction` is exactly to avoid null
`CompactionStatus eligibility`, after this PR, the `CompactionCandidate` would
always have an eligibility.
don't we need the uncompacted & compacted stats in `CompactionStatus` to
sort `CompactionCandidate`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
