-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
handle overflow for MutableOffHeapByteArrayStore
buffer starting size
#13215
handle overflow for MutableOffHeapByteArrayStore
buffer starting size
#13215
Conversation
625457a
to
0779406
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having 2GB as start size doesn't look correct. Can you check the high level logic and see if this is expected? Seems like we are trying to use one single buffer to hold everything?
...rc/main/java/org/apache/pinot/segment/local/io/writer/impl/MutableOffHeapByteArrayStore.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13215 +/- ##
============================================
+ Coverage 61.75% 62.03% +0.28%
+ Complexity 207 198 -9
============================================
Files 2436 2534 +98
Lines 133233 139321 +6088
Branches 20636 21535 +899
============================================
+ Hits 82274 86434 +4160
- Misses 44911 46401 +1490
- Partials 6048 6486 +438
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Looks like we're hitting an edge case. The contributing factors are:
Together they can result in the estimated size based on I think the generic solution is to allow MV columns to be raw encoded even in the mutable segment. We could also use I'm not sure either of these should be in the scope of this PR. What do you think? |
@@ -170,15 +170,20 @@ public void close() | |||
private final int _startSize; | |||
|
|||
@VisibleForTesting | |||
public int getStartSize() { | |||
return _startSize; | |||
public static int getStartSize(int numArrays, int avgArrayLen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made static to avoid initializing huge buffers in unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to mix the scope of this PR and the general solution. The fix here is valid
We've seen some state transition failrues from
OFFLINE -> CONSUMING
with the below stack trace. It looks like_startSize
overflow isn't handled, so this change handles it.It also treats
0
as an overflow case during buffer expansion, since that would hit the same precondition check as well.tag:
bugfix