Resolve partitionId overflow issue when number of segments exceeds Java Short.MAX_VALUE in single time period

[dulu98Kurz]

Fixes #15091.

Description

This PR address the issue described in #15091
You can also find discussion in #15090 for detailed discussion for this PR.
This PR should resolve the issue more extensively, we should merge this one if no other concerns.

Fixed the bug ...

#15091

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Resolve partitionId overflow issue when number of segments exceeds Java Short.MAX_VALUE in single time period.

---

Key changed/added classes in this PR

• MyFoo
• OurBar
• TheirBaz

---

This PR has:

• been self-reviewed.
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• 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
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[dulu98Kurz]

the CI test failures seems related with MSQ instead of this PR.

[cryptoe]

@dulu98Kurz I have left an initial review.
Would request @kfaraz to take a look into this since he has worked on this AREA of druid.

[dulu98Kurz]

@cryptoe @kfaraz Updated as per comments, agreed it would be nice to include interval info when throwing exception or logging, but I can't find a path that can expose interval to RootPartitionRange easily, please let me know if you have suggestion about this!

[cryptoe]

@dulu98Kurz Thanks for incorporating some feedback. @kfaraz had some doubts around the increased memory pressure on overshadowable manager due to change to int from short.

I kind of agree with his assessment. I think we should keep the error message handling and revert the change from int to short.
With concurrent compaction coming in, I think this situation is likely to happen less frequently.
cc @kfaraz .

[AmatyaAvadhanula]

Hello @dulu98Kurz

#7491 - Segment locking requires the partition ids to be within 0 - Short.MAX_VALUE. In particular:

Regarding restrictions above, it's important to make the partitionIds of the first-generation segments consecutive to facilitate compaction. To do this, we use separate partitionId space for first-generation segments and non-first-generation segments: (0 - 32767) is for first-generation and (32768 - 65535) is for non-first-generation segments.

However, there are plans to deprecate segment locking in future releases in favour of concurrent append with replace. We might be able to revisit this then.

[dulu98Kurz]

Hi @cryptoe @kfaraz @AmatyaAvadhanula thanks a lot for the input, seems there are more than one concerns on refactoring to int I have reverted related change.

However I believe I found a way to make everyone happy without refactoring to int, remember the origination of issue #15091 is:
2023-09-29T02:45:16,308 ERROR [task-runner-0-priority-0] org.apache.druid.indexing.seekablestream.SeekableStreamIndexTaskRunner - Encountered exception while running task. java.lang.IllegalArgumentException: fromKey > toKey at java.util.TreeMap$NavigableSubMap.<init>(TreeMap.java:1368) ~[?:1.8.0_302] at java.util.TreeMap$AscendingSubMap.<init>(TreeMap.java:1855) ~[?:1.8.0_302] at java.util.TreeMap.subMap(TreeMap.java:913) ~[?:1.8.0_302] at org.apache.druid.timeline.partition.OvershadowableManager.entryIteratorGreaterThan(OvershadowableManager.java:423) ~[druid-processing-2023.03.1-iap.jar:2023.03.1-iap]

Now since we heavily relies on Short.toUnsignedInt when comparing and determining overlapping, we can make sure we never encounter java.lang.IllegalArgumentException: fromKey > toKey by setting the toKey to 0xFFFF instead of Short.MAX_VALUE, and 0xFFFF is -1 in signed short, so instead of doing:
final RootPartitionRange highFence = new RootPartitionRange(Short.MAX_VALUE, Short.MAX_VALUE);
We do :
final RootPartitionRange highFence = new RootPartitionRange((short) (-1), (short) (-1));
To show a few examples that illustrate why -1 works:

This change should address the logic mistakes in the code and keep ingestion task alive, without increasing memory footprint, please advise! Thanks again for the time on this!

[dulu98Kurz]

Hi @cryptoe @kfaraz @AmatyaAvadhanula does the change above make sense to you? I guess the only change I'm making here is this line https://github.com/apache/druid/pull/15116/files#diff-9eda353464cdf927f17c657f317fc7986902ab9eb20d99a951d23b6435fc16beR427 , I`m happy to post more clarifications if any further questions

[AmatyaAvadhanula]

Hello @dulu98Kurz
It appears that the proposed change is to increase the limit from 32767 to 65535. If that is the case, segment locking would still break as ids from 32767 to 65535 are reserved for other purposes.

The changes to improve the error message look good to me though.

[kfaraz]

@dulu98Kurz , thanks for attempting to fix this!

As @AmatyaAvadhanula points out, there are other concerns in just changing this short to int. Similarly, truncating the values to ensure that overflow never happens is not the right approach either.

Issue #15356 has been created to discuss a more long-term solution for this.

For now, I think the best approach for your PR would be to ensure fail fast in such cases and give a better error message.

[kfaraz]

For now, I think the best approach for your PR would be to ensure fail fast in such cases and give a better error message.

This PR can be closed as PR #15685 has already addressed the issue of updating the error message.
The original problem will be tackled as a fix to #15356.

[dulu98Kurz]

Sorry for the late response @kfaraz , since the issue is addressed in another PR I`ll close this one, thanks for your attention and looking forward to collaborate again in future!