cryptoe commented on code in PR #15116:
URL: https://github.com/apache/druid/pull/15116#discussion_r1368373371
##########
processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java:
##########
@@ -399,13 +395,20 @@ boolean isOvershadowedByVisibleGroup(RootPartitionRange
partitionRange, short mi
* A RootPartitionRange is smaller than a partitionId if {@link
RootPartitionRange#startPartitionId} < partitionId.
*/
private Iterator<Entry<RootPartitionRange,
Short2ObjectSortedMap<AtomicUpdateGroup<T>>>> entryIteratorSmallerThan(
- short partitionId,
+ int partitionId,
TreeMap<RootPartitionRange, Short2ObjectSortedMap<AtomicUpdateGroup<T>>>
stateMap
)
{
- final RootPartitionRange lowFench = new RootPartitionRange((short) 0,
(short) 0);
+ final RootPartitionRange lowFence = new RootPartitionRange(0, 0);
final RootPartitionRange highFence = new RootPartitionRange(partitionId,
partitionId);
- return stateMap.subMap(lowFench, false, highFence,
false).descendingMap().entrySet().iterator();
+ Iterator<Entry<RootPartitionRange,
Short2ObjectSortedMap<AtomicUpdateGroup<T>>>> subItr;
+ try {
+ subItr = stateMap.subMap(lowFence, false, highFence,
false).descendingMap().entrySet().iterator();
+ }
+ catch (IllegalArgumentException e) {
+ throw new IAE("Illegal partitionId %s", highFence);
Review Comment:
We should throw `DruidException.defensive` here.
You can read about the druid way of exceptions :
https://github.com/apache/druid/pull/14004
##########
processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java:
##########
@@ -414,13 +417,20 @@ private Iterator<Entry<RootPartitionRange,
Short2ObjectSortedMap<AtomicUpdateGro
* and {@link RootPartitionRange#endPartitionId} > partitionId.
*/
private Iterator<Entry<RootPartitionRange,
Short2ObjectSortedMap<AtomicUpdateGroup<T>>>> entryIteratorGreaterThan(
- short partitionId,
+ int partitionId,
TreeMap<RootPartitionRange, Short2ObjectSortedMap<AtomicUpdateGroup<T>>>
stateMap
)
{
- final RootPartitionRange lowFench = new RootPartitionRange(partitionId,
partitionId);
- final RootPartitionRange highFence = new
RootPartitionRange(Short.MAX_VALUE, Short.MAX_VALUE);
- return stateMap.subMap(lowFench, false, highFence,
false).entrySet().iterator();
+ final RootPartitionRange lowFence = new RootPartitionRange(partitionId,
partitionId);
+ final RootPartitionRange highFence = new
RootPartitionRange(Integer.MAX_VALUE, Integer.MAX_VALUE);
+ Iterator<Entry<RootPartitionRange,
Short2ObjectSortedMap<AtomicUpdateGroup<T>>>> subItr;
+ try {
+ subItr = stateMap.subMap(lowFence, false, highFence,
false).entrySet().iterator();
+ }
+ catch (IllegalArgumentException e) {
+ throw new IAE("Illegal partitionId %s, please reduce number of segments
per time-trunk!", lowFence);
Review Comment:
Similar comment here
##########
processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java:
##########
@@ -1033,13 +1039,21 @@ public String toString()
@VisibleForTesting
static class RootPartitionRange implements Comparable<RootPartitionRange>
{
- private final short startPartitionId;
- private final short endPartitionId;
+ private final int startPartitionId;
+ private final int endPartitionId;
+ private static final int MAX_PARTITIONS_PER_TIMETRUNK = 65536;
@VisibleForTesting
static RootPartitionRange of(int startPartitionId, int endPartitionId)
{
- return new RootPartitionRange((short) startPartitionId, (short)
endPartitionId);
+ if (startPartitionId > MAX_PARTITIONS_PER_TIMETRUNK) {
+ log.error(
+ "PartitionId [%s] exceeding max number of Segments Druid can
handle in single time-trunk [%s], please compact or reduce number of segments
in time-trunk!",
+ startPartitionId,
+ MAX_PARTITIONS_PER_TIMETRUNK
+ );
+ }
Review Comment:
```suggestion
if (startPartitionId > MAX_PARTITIONS_PER_TIMETRUNK) {
log.error(
"PartitionId [%s] exceeding max number of Segments Druid can
handle in single time-chunk [%s], please compact or reduce number of segments
in time-chunk!",
startPartitionId,
MAX_PARTITIONS_PER_TIMECHUNK
);
}
```
Also we should throw a DruidException for persona user. .
##########
processing/src/test/java/org/apache/druid/timeline/partition/OvershadowableManagerTest.java:
##########
@@ -165,6 +165,35 @@ public void testFindOvershadowedBy()
);
}
+ @Test
+ public void testHandleOutOfRangeFindOvershadowedBy()
+ {
+ final List<PartitionChunk<OvershadowableInteger>>
expectedOvershadowedChunks = new ArrayList<>();
+ PartitionChunk<OvershadowableInteger> chunk = newNonRootChunk(32001,
32003, 1, 1);
+ manager.addChunk(chunk);
+ expectedOvershadowedChunks.add(chunk);
Review Comment:
Lets add a test case which is one more than `MAX_PARTITIONS_PER_TIMECHUNK`
as well.
--
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]
