kfaraz commented on code in PR #19975:
URL: https://github.com/apache/druid/pull/19975#discussion_r3760252486
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -272,6 +277,16 @@ public static CompactionStatus skipped(String
reasonFormat, Object... args)
return new CompactionStatus(State.SKIPPED,
StringUtils.format(reasonFormat, args), null, null, null);
}
+ /**
+ * Denotes an interval that needs compaction but has been filtered out by the
+ * {@link CompactionCandidateSearchPolicy}. Unlike {@link #skipped}, such an
+ * interval becomes compactible again if the policy thresholds are relaxed.
+ */
+ public static CompactionStatus policyExcluded(String reasonFormat, Object...
args)
Review Comment:
```suggestion
public static CompactionStatus excludedByPolicy(String reasonFormat,
Object... args)
```
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -129,6 +129,11 @@ public boolean isSkipped()
return state == State.SKIPPED;
}
+ public boolean isPolicyExcluded()
Review Comment:
```suggestion
public boolean isExcludedByPolicy()
```
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -65,7 +65,7 @@ public class CompactionStatus
public enum State
{
- COMPLETE, PENDING, RUNNING, SKIPPED
+ COMPLETE, PENDING, RUNNING, SKIPPED, POLICY_EXCLUDED
Review Comment:
I originally wanted `SKIPPED` to cover all skip reasons, and have the reason
message itself be the distinguishing factor between things like `excluded by
policy`, `interval locked by another task`, `interval lies in skip offset`.
Over time, we might want to be able to identify the interval/segment/byte
counts against all of these reasons.
How do you feel about the following instead:
- Do not add another value to the `State` enum
- Have a new enum or maybe a bunch of constant Strings (aka error codes)
which will serve as the skip reason
- Have the compaction snapshot maintain counts for different skip reasons
instead and just include that in the report.
##########
indexing-service/src/main/java/org/apache/druid/indexing/compact/CompactionConfigBasedJobTemplate.java:
##########
@@ -99,6 +100,11 @@ public List<CompactionJob> createCompactionJobs(
.getCompactionPolicy()
.checkEligibilityForCompaction(candidate,
params.getLatestTaskStatus(candidate));
if (!eligibility.isEligible()) {
+ params.getSnapshotBuilder().addToPolicyExcluded(
+ candidate.withCurrentStatus(
+ CompactionStatus.policyExcluded("Rejected by search policy:
%s", eligibility.getReason())
Review Comment:
We can omit the prefixed message `Rejected by search policy` since the state
`POLICY_EXCLUDED` already implies that.
##########
server/src/main/java/org/apache/druid/server/coordinator/AutoCompactionSnapshot.java:
##########
@@ -51,17 +51,23 @@ public enum ScheduleStatus
@JsonProperty
private final long bytesSkipped;
@JsonProperty
+ private final long bytesPolicyExcluded;
+ @JsonProperty
private final long segmentCountAwaitingCompaction;
@JsonProperty
private final long segmentCountCompacted;
@JsonProperty
private final long segmentCountSkipped;
@JsonProperty
+ private final long segmentCountPolicyExcluded;
Review Comment:
Nit: Might be nicer to have these fields and methods read as
`xyzExcludedByPolicy`.
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -65,7 +65,7 @@ public class CompactionStatus
public enum State
{
- COMPLETE, PENDING, RUNNING, SKIPPED
+ COMPLETE, PENDING, RUNNING, SKIPPED, POLICY_EXCLUDED
Review Comment:
I originally wanted `SKIPPED` to cover all skip reasons, and have the reason
message itself be the distinguishing factor between things like `excluded by
policy`, `interval locked by another task`, `interval lies in skip offset`.
Over time, we might want to be able to identify the interval/segment/byte
counts against all of these reasons.
How do you feel about the following instead:
- Do not add another value to the `State` enum
- Have a new enum or maybe a bunch of constant Strings (aka error codes)
which will serve as the skip reason
- Have the compaction snapshot maintain counts for different skip reasons
instead and just include that in the report.
- Emit the skip reason as a dimension against the
`segment/skipCompact/bytes` and other metrics
--
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]
