capistrant commented on code in PR #18968:
URL: https://github.com/apache/druid/pull/18968#discussion_r2788628771
##########
server/src/main/java/org/apache/druid/server/coordinator/duty/CompactSegments.java:
##########
@@ -371,8 +369,11 @@ public static ClientCompactionTaskQuery
createCompactionTask(
}
final Map<String, Object> autoCompactionContext =
newAutoCompactionContext(config.getTaskContext());
- if (candidate.getCurrentStatus() != null) {
- autoCompactionContext.put(COMPACTION_REASON_KEY,
candidate.getCurrentStatus().getReason());
+ if (CompactionMode.NOT_APPLICABLE.equals(candidate.getMode())) {
Review Comment:
Is this supposed to be `if
(!CompactionMode.NOT_APPLICABLE.equals(candidate.getMode()))`?
Seems to me like for compaction jobs the reason will be missing now, and for
NOT_APPLICABLE a task wouldn't even be started anyways, as an exception would
get thrown by `compactSegments`
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionMode.java:
##########
Review Comment:
I think some javadocs in this class would be helpful. A class level one + at
least a brief sentence of each mode, even if they are a bit self explanatory
##########
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:
will this make it less clear what is causing the difference than having all
the individual assertions? your way looks better on the eyes, but I wonder if
it makes it more annoying for a dev to identify what exactly they broke if it
starts failing?
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -346,8 +361,10 @@ static DimensionRangePartitionsSpec
getEffectiveRangePartitionsSpec(DimensionRan
*/
private static class Evaluator
Review Comment:
I'm not sure how much I like moving away from a more descriptive return type
for the individual checks. It feels like a nitpick since functionally
everything is the same. And I guess I understand why you wanted to not be using
`CompactionStatus` all over the place in the Evaluator like we were. My only
alternative suggestion would be yet another simple record like object or
something signifying a check with a result and reason (populated if the check
failed). The "return null if check passed" just feels a little unintuitive.
Maybe a compromise would be explicitly stating how the checks work in javadoc
for the CHECKS and FINGERPRINT_CHECKS lists (or in the evalaute method javadoc)
##########
server/src/main/java/org/apache/druid/server/compaction/CompactionStatus.java:
##########
@@ -357,29 +348,52 @@ private static class Evaluator
private final Map<CompactionState, List<DataSegment>>
unknownStateToSegments = new HashMap<>();
@Nullable
- private final String targetFingerprint;
private final IndexingStateFingerprintMapper fingerprintMapper;
+ @Nullable
+ private final String targetFingerprint;
- private Evaluator(
+ Evaluator(
CompactionCandidate candidateSegments,
DataSourceCompactionConfig compactionConfig,
- @Nullable String targetFingerprint,
@Nullable IndexingStateFingerprintMapper fingerprintMapper
)
{
this.candidateSegments = candidateSegments;
this.compactionConfig = compactionConfig;
this.tuningConfig =
ClientCompactionTaskQueryTuningConfig.from(compactionConfig);
this.configuredGranularitySpec = compactionConfig.getGranularitySpec();
- this.targetFingerprint = targetFingerprint;
this.fingerprintMapper = fingerprintMapper;
+ if (fingerprintMapper == null) {
+ targetFingerprint = null;
+ } else {
+ targetFingerprint = fingerprintMapper.generateFingerprint(
+ compactionConfig.getDataSource(),
+ compactionConfig.toCompactionState()
+ );
+ }
+ }
+
+ List<DataSegment> getUncompactedSegments()
+ {
+ return uncompactedSegments;
}
- private CompactionStatus evaluate()
+ /**
+ * Evaluates the compaction status of candidate segments through a
multi-step process:
+ * <ol>
+ * <li>Validates input bytes are within limits</li>
+ * <li>Categorizes segments by compaction state (fingerprinted,
uncompacted, or unknown)</li>
+ * <li>Performs fingerprint-based validation if available (fast
path)</li>
+ * <li>Runs detailed checks against unknown states via {@link
#CHECKS}</li>
+ * </ol>
+ *
+ * @return Pair of eligibility status and compaction status with reason
for first failed check
+ */
+ Pair<CompactionCandidateSearchPolicy.Eligibility, CompactionStatus>
evaluate()
{
- final CompactionStatus inputBytesCheck = inputBytesAreWithinLimit();
- if (inputBytesCheck.isSkipped()) {
- return inputBytesCheck;
+ final CompactionCandidateSearchPolicy.Eligibility inputBytesCheck =
inputBytesAreWithinLimit();
+ if (inputBytesCheck != null) {
Review Comment:
I guess this conversation is out of date after latest refactorings?
--
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]
