-
Notifications
You must be signed in to change notification settings - Fork 964
fix: throw reject when SingleThreadExecutor drainTo in progress and queue is empty #4488
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
fix: throw reject when SingleThreadExecutor drainTo in progress and queue is empty #4488
Conversation
cd0eeb9 to
3de037f
Compare
|
@lhotari Could you have a chance to review? |
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SingleThreadExecutor.java
Outdated
Show resolved
Hide resolved
StevenLuMT
left a comment
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.
Boundary value confirmation is required
f0c9bb2 to
7785861
Compare
StevenLuMT
left a comment
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.
LGTM
xiezhx9
left a comment
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.
squash into one commit might be better
The code owner can use squash merge to resolve this. |
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SingleThreadExecutor.java
Show resolved
Hide resolved
b35429c to
aaafec6
Compare
jiazhai
left a comment
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.
+1. LTGM. Good Catch.
Fix #4465
Motivaction
In
SingleThreadExecutor, therunnerdrains all tasks from thequeueintolocalTasks. Although thequeueis empty in memory at this point, the tasks are still pending execution inlocalTasks—so logically, the queue is still "full."Calling
execute()during this phase should not enqueue a newrunnableinto thequeue, as doing so would exceed the intended capacity. This can lead to increased memory usage and potential OutOfMemory (OOM) issues.Changes
To address this, we introduce a variable to track the total number of pending runnables. This counter is used to control whether a new task should be added:
The counter is incremented when a runnable is added to the queue.
The counter is decremented when a runnable is actually executed.
This ensures accurate tracking of pending tasks and prevents overfilling the logical task queue.