-
Notifications
You must be signed in to change notification settings - Fork 963
fix replicateRate of batchRead in auto-recover is negative #4638
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
base: master
Are you sure you want to change the base?
fix replicateRate of batchRead in auto-recover is negative #4638
Conversation
|
good jobs, I have a question: should maxBytesToReplicate use long type? |
rateLimiter.acquire(int) just support int value. Besides, Integer.max_value means 2GB throughput, it is enough for most situation. |
|
| int bytesToReplicateCnt = averageEntrySize.get() * entriesToReplicateCnt; | ||
| bytesToReplicateCnt = bytesToReplicateCnt >= 0 ? bytesToReplicateCnt : Integer.MAX_VALUE; | ||
| maxBytesToReplicate = Math.min(maxBytesToReplicate, bytesToReplicateCnt); |
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.
I think making this calculation with a long value would produce more correct results.
It's not really realistic, but it I guess it's possible that the resulting value could "wrap around" the negative range and become positive.
Something like this:
| int bytesToReplicateCnt = averageEntrySize.get() * entriesToReplicateCnt; | |
| bytesToReplicateCnt = bytesToReplicateCnt >= 0 ? bytesToReplicateCnt : Integer.MAX_VALUE; | |
| maxBytesToReplicate = Math.min(maxBytesToReplicate, bytesToReplicateCnt); | |
| long bytesToReplicateCnt = (long) averageEntrySize.get() * entriesToReplicateCnt; | |
| maxBytesToReplicate = bytesToReplicateCnt > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int) bytesToReplicateCnt; |
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.
This modification is incorrect. maxBytesToReplicate still may be negative if averageEntrySize.get() is negative, and then replicationThrottle.acquire(maxBytesToReplicate) is stuck.
I think we should "wrap around" the negative value and make it become positive. We should keep the bookie availability first.
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.
can averageEntrySize.get() be negative?
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.
I think we should "wrap around" the negative value and make it become positive.
The point is that such value could be positive, but it wouldn't match a meaningful value.
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.
For example 4294967297 would be just 1 with a "wrapped around" value.
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.
I get your point. you are right. 4294967297 is a corner case for int value.
Make it to long value would be better.
e21eb64 to
709bef6
Compare
Motivation
Currently the replicateRate of batchRead in auto-recover may exceed Integer.MAX_VALUE and then become negative, which would result in batchRead operation stuck.
Changes
deal with the negative situation