LakshSingla commented on code in PR #15243:
URL: https://github.com/apache/druid/pull/15243#discussion_r1381193342
##########
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:
We can pull out the exception above.
`IAE` to me is more like entering a negative integer instead of a negative
one, or a different type. The logic inside the helper operates before comparing
it with the maxBuckets parameter. Or perhaps there are "XOR args" (enter
exactly one) and the user passes both.
As per InsertLockPreempted, it might be insufficiency at the time when it
was written, but ISE seems more forgiving than IAE. Also, that logic in itself
is also pretty fragile, if the logic switches to `DruidException`, then it
would break.
At worst, IAE can have some special text inside, and we do a string compare
before throwing this exception, however since we already have a special
exception created, that seems like a valid path to follow
##########
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:
We can pull out the exception above if that helps
`IAE` to me is more like entering a negative integer instead of a negative
one, or a different type. The logic inside the helper operates before comparing
it with the maxBuckets parameter. Or perhaps there are "XOR args" (enter
exactly one) and the user passes both.
As per InsertLockPreempted, it might be insufficiency at the time when it
was written, but ISE seems more forgiving than IAE. Also, that logic in itself
is also pretty fragile, if the logic switches to `DruidException`, then it
would break.
At worst, IAE can have some special text inside, and we do a string compare
before throwing this exception, however since we already have a special
exception created, that seems like a valid path to follow
--
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]
