dulu98Kurz commented on code in PR #15090:
URL: https://github.com/apache/druid/pull/15090#discussion_r1350650766
##########
processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java:
##########
@@ -418,9 +420,11 @@ private Iterator<Entry<RootPartitionRange,
Short2ObjectSortedMap<AtomicUpdateGro
TreeMap<RootPartitionRange, Short2ObjectSortedMap<AtomicUpdateGroup<T>>>
stateMap
)
{
- final RootPartitionRange lowFench = new RootPartitionRange(partitionId,
partitionId);
+ // remediate submap `fromKey > toKey` issue when partitionId overflows
+ final short partitionIdLowFence = partitionId < 0 ? Short.MAX_VALUE :
partitionId;
Review Comment:
From code quality perspective, `Short.toUnsignedInt` is a precision-loss
conversion and we used it in 2 files for 18 times, we can simplify the logic
and improve readability if we change to `int`
<img width="606" alt="image"
src="https://github.com/apache/druid/assets/14854898/47a36432-d6fb-4f7e-9fdf-1610bf54e39b";>
Lastly, when partitionId is out of range, the
[logic](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/timeline/partition/OvershadowableManager.java#L421-L423)
we use to handle it right now is simply wrong:
```
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();
```
For example when:
<img width="304" alt="image"
src="https://github.com/apache/druid/assets/14854898/03d6e2c1-ed5d-4e10-b708-e05699fb12f2";>
And `stateMap.subMap(lowFench, false, highFence, false)` will return all
entries instead of empty ...
If we are ok with remediation in this PR, we can proceed with merging, if we
are OK with refactoring please allow me to send another PR to fix it more
completely.
--
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]
