You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/03/18 01:17:04 UTC
[GitHub] [druid] maytasm opened a new pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
maytasm opened a new pull request #11009:
URL: https://github.com/apache/druid/pull/11009
Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
### Description
Auto-compaction now supports setting SegmentGranularity. However, it currently submits compaction tasks unnecessary for segments that was compacted (lastCompactionState != null) but lastCompactionState.getGranularitySpec() == null (i.e. compacted when the auto compaction spec does not set SegmentGranularity) and the segments' SegmentGranularity is already the same as the SegmentGranularity configured in the auto compaction spec. This change will make auto compaction checks the existing segments' SegmentGranularity and determine if compaction is needed or not even when SegmentGranularity is not stored in lastCompactionState.
Note that, with this change, we actually never need to check SegmentGranularity in lastCompactionState to determine if segments need compaction or not. However, I still keep that check there for the following reasons:
1) If lastCompactionState has SegmentGranularity, then it is faster to check and compare using that.
2) If we add support for queryGranularity, we will need to check it in lastCompactionState
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] added integration tests.
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm merged pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #11009:
URL: https://github.com/apache/druid/pull/11009
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597276407
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,34 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
Review comment:
`GranularityType.fromPeriod()` can throw an exception if the segment interval is not one of the enums in `GranularityType`. I think we should probably compare the duration of the granularity as well as their timezone and origin.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597224189
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,29 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
+ .collect(Collectors.toSet());
+ if (segmentGranularities.size() != 1 || !segmentGranularities.contains(config.getGranularitySpec().getSegmentGranularity())) {
+ needsCompaction = true;
+ }
+ } else if (!config.getGranularitySpec().getSegmentGranularity().equals(existingSegmentGranularity)) {
+ log.info(
+ "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597421983
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,34 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
Review comment:
Done. I think this should work
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597861927
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +397,24 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity as the configured segment granularity.
+ needsCompaction = candidates.segments.stream()
+ .anyMatch(segment -> !config.getGranularitySpec().getSegmentGranularity().isAligned(segment.getInterval()));
Review comment:
Do you want to print a log in this case too?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597224953
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,29 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
+ .collect(Collectors.toSet());
+ if (segmentGranularities.size() != 1 || !segmentGranularities.contains(config.getGranularitySpec().getSegmentGranularity())) {
+ needsCompaction = true;
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r596606358
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,29 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
+ .collect(Collectors.toSet());
+ if (segmentGranularities.size() != 1 || !segmentGranularities.contains(config.getGranularitySpec().getSegmentGranularity())) {
+ needsCompaction = true;
Review comment:
do we need a log statement here as well?
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,29 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
+ .collect(Collectors.toSet());
+ if (segmentGranularities.size() != 1 || !segmentGranularities.contains(config.getGranularitySpec().getSegmentGranularity())) {
+ needsCompaction = true;
+ }
+ } else if (!config.getGranularitySpec().getSegmentGranularity().equals(existingSegmentGranularity)) {
+ log.info(
+ "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
Review comment:
nit: maybe you can just log the segment granularity instead of the granularity spec?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597278752
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,34 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
Review comment:
Should we just catch the exception and set needsCompaction to true in that case?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597874091
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +397,24 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity as the configured segment granularity.
+ needsCompaction = candidates.segments.stream()
+ .anyMatch(segment -> !config.getGranularitySpec().getSegmentGranularity().isAligned(segment.getInterval()));
Review comment:
Thanks for the review. I accidentally removed the logging message. Added it back in.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11009: Auto-compaction with segment granularity should skip segments that already have the configured segmentGranularity
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11009:
URL: https://github.com/apache/druid/pull/11009#discussion_r597279206
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -397,20 +398,34 @@ private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCom
needsCompaction = true;
}
- // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
- final Granularity segmentGranularity = lastCompactionState.getGranularitySpec() != null ?
- objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
- null;
-
- if (config.getGranularitySpec() != null &&
- config.getGranularitySpec().getSegmentGranularity() != null &&
- !config.getGranularitySpec().getSegmentGranularity().equals(segmentGranularity)) {
- log.info(
- "Configured granularitySpec[%s] is different from the one[%s] of segments. Needs compaction",
- config.getGranularitySpec(),
- segmentGranularity
- );
- needsCompaction = true;
+ if (config.getGranularitySpec() != null && config.getGranularitySpec().getSegmentGranularity() != null) {
+ // Only checks for segmentGranularity as auto compaction currently only supports segmentGranularity
+ final Granularity existingSegmentGranularity = lastCompactionState.getGranularitySpec() != null ?
+ objectMapper.convertValue(lastCompactionState.getGranularitySpec(), GranularitySpec.class).getSegmentGranularity() :
+ null;
+ if (existingSegmentGranularity == null) {
+ // Candidate segments were all compacted without segment granularity set.
+ // We need to check if all segments have the same segment granularity and if it is the same
+ // as the configured segment granularity.
+ Set<Granularity> segmentGranularities = candidates.segments.stream()
+ .map(segment -> GranularityType.fromPeriod(segment.getInterval().toPeriod()).getDefaultGranularity())
Review comment:
Since that means it must not be the same as the configured segmentGranularity in the auto compaction spec. Since it if it is the same, then it must be resolvable to a Granularity
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org