Skip to content
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

The maxPendingAddRequestsPerThread configuration is inconsistent with the actual behavior, resulting in netty direct memory OOM #4465

Open
AuroraTwinkle opened this issue Jul 18, 2024 · 3 comments · May be fixed by #4488
Assignees
Labels

Comments

@AuroraTwinkle
Copy link
Contributor

BUG REPORT

Describe the bug

The maxPendingAddRequestsPerThread configuration in bookkeeper.conf is inconsistent with the actual behavior, resulting in netty direct memory OOM.

To Reproduce

Steps to reproduce the behavior:

  1. set config: maxPendingAddRequestsPerThread=1000
  1. simulate rocksdb failure, flush is blocked for a long time
  2. addEntry requests are queued in the thread pool, and the size of queues is twice the maxPendingAddRequestsPerThread configuration
  3. the size of queued requests doubled, resulting in netty direct memory OOM

Expected behavior

If we configure maxPendingAddRequestsPerThread to 1000, the maximum number of thread pool queues should not exceed 1000, otherwise it will cause unexpected memory usage and may cause OOM.

By reading the source code, I found that the root cause of the problem is that there is a localTasks logic in SingleThreadExecutor. Before each execution, it tries to drain all tasks from the thread pool queue to localTasks. At this time, the thread pool queue is equivalent to being cleared, but the tasks in localTasks have not yet been executed. In extreme cases, the number of tasks queued by each SingleThreadExecutor will double. I guess the purpose of using localTasks is to avoid lock contention and improve performance, which is fine. But perhaps we should consider solving the problem of doubling the number of queued tasks.

Screenshots
image
image
image

Additional context

Add any other context about the problem here.

@AuroraTwinkle
Copy link
Contributor Author

@merlimat Hi, please pay attention to this issue, thanks!

@nodece
Copy link
Member

nodece commented Aug 23, 2024

It seems that we need to add an atomic integer to record the working task.

@nodece
Copy link
Member

nodece commented Aug 24, 2024

I submitted a PR to fix this issue: #4488

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants