LakshSingla commented on code in PR #15243:
URL: https://github.com/apache/druid/pull/15243#discussion_r1380638744
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java:
##########
@@ -173,66 +190,69 @@ public Set<DataSegment>
computeTombstoneSegmentsForReplace(
* They should be aligned with the
replaceGranularity
* @param dataSource Datasource on which the replace is to be
performed
* @param replaceGranularity Granularity of the replace query
+ * @param maxBuckets Maximum number of partition buckets. If the
number of computed tombstone buckets
+ * exceeds this threshold, the method will throw
an error.
* @return Intervals computed for the tombstones
* @throws IOException
*/
public Set<Interval> computeTombstoneIntervalsForReplace(
List<Interval> intervalsToDrop,
List<Interval> intervalsToReplace,
String dataSource,
- Granularity replaceGranularity
+ Granularity replaceGranularity,
+ int maxBuckets
) throws IOException
{
Set<Interval> retVal = new HashSet<>();
List<Interval> usedIntervals =
getExistingNonEmptyIntervalsOfDatasource(intervalsToReplace, dataSource);
+ int buckets = 0;
for (Interval intervalToDrop : intervalsToDrop) {
for (Interval usedInterval : usedIntervals) {
Interval overlap = intervalToDrop.overlap(usedInterval);
- // No overlap of the dropped segment with the used interval due to
which we donot need to generate any tombstone
+ // No overlap of the dropped segment with the used interval due to
which we do not need to generate any tombstone
if (overlap == null) {
continue;
}
+ if (buckets > maxBuckets) {
+ throw new IAE("Cannot add more tombstone buckets than [%d].",
maxBuckets);
Review Comment:
There's a `TooManyBucketsException` which seems more appropriate here.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java:
##########
@@ -985,13 +1041,61 @@ public void
testReplaceTombstonesOverPartiallyOverlappingSegments()
.setExpectedShardSpec(DimensionRangeShardSpec.class)
.setExpectedTombstoneIntervals(
ImmutableSet.of(
- Intervals.of("2001-04-01/2002-01-01")
+ Intervals.of("2001-04-01/P3M"),
+ Intervals.of("2001-07-01/P3M"),
+ Intervals.of("2001-10-01/P3M")
)
)
.setExpectedResultRows(expectedResults)
.verifyResults();
}
+ @Test
+ public void testReplaceTombstonesWithTooManyBucketsThrowsException()
Review Comment:
`MSQFaultsTest` is a more apt place for this test.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -1423,14 +1424,18 @@ private void publishAllSegments(final Set<DataSegment>
segments) throws IOExcept
intervalsToDrop,
destination.getReplaceTimeChunks(),
task.getDataSource(),
- destination.getSegmentGranularity()
+ destination.getSegmentGranularity(),
+ Limits.MAX_PARTITION_BUCKETS
);
segmentsWithTombstones.addAll(tombstones);
numTombstones = tombstones.size();
}
catch (IllegalStateException e) {
throw new MSQException(e, InsertLockPreemptedFault.instance());
}
+ catch (IllegalArgumentException e) {
+ throw new MSQException(e, new
TooManyBucketsFault(Limits.MAX_PARTITION_BUCKETS));
+ }
Review Comment:
I think this will also change, IAE doesn't seem valid here.
--
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]
