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

Fix a potential bug scheduling unnecessary threads #6104

Closed

Conversation

riversand963
Copy link
Contributor

Summary:
RocksDB should decrement the counter unscheduled_flushes_ as soon as the bg
thread is scheduled. Before this fix, the counter is decremented only when the
bg thread starts and picks an element from the flush queue. This may cause more
than necessary bg threads to be scheduled. Not a correctness issue, but may
affect flush thread count.

Test Plan:

make check

Summary:
RocksDB should decrement the counter `unscheduled_flushes_` as soon as the bg
thread is scheduled. Before this fix, the counter is decremented only when the
bg thread starts and picks an element from the flush queue. This may cause more
than necessary bg threads to be scheduled. Not a correctness issue, but may
affect flush thread count.

Test Plan:
```
make check
```
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you remember why it was moved in the first place?

@riversand963
Copy link
Contributor Author

Do you remember why it was moved in the first place?

@siying
I think it was moved to the bg flush thread because we introduced FlushRequest that can contain multiple column families. Each time we request a flush, we increase unscheduled_flushes_ by flush_request.size(). Consequently, each time we need to decrease it by flush_request.size(). Therefore, we need to peek the flush queue to determine the flush request size. In the bg thread, this information is readily available, thus I mistakenly moved it there.

@riversand963 riversand963 deleted the dec_unflushed_counter branch November 27, 2019 22:51
@facebook-github-bot
Copy link
Contributor

@riversand963 merged this pull request in 09fcf4f.

wolfkdy pushed a commit to wolfkdy/rocksdb that referenced this pull request Dec 22, 2019
Summary:
RocksDB should decrement the counter `unscheduled_flushes_` as soon as the bg
thread is scheduled. Before this fix, the counter is decremented only when the
bg thread starts and picks an element from the flush queue. This may cause more
than necessary bg threads to be scheduled. Not a correctness issue, but may
affect flush thread count.
Pull Request resolved: facebook#6104

Test Plan:
```
make check
```

Differential Revision: D18735584

Pulled By: riversand963

fbshipit-source-id: d36272d4a08a494aeeab6200a3cff7a3d1a2dc10
junhanLee95 added a commit to junhanLee95/rocksdb that referenced this pull request Jun 28, 2023
this fix is due to facebook#6104.
unscheduled_flushes_ was not handled well in the current version (v6.1.2.)
This leads flush_scheduled_ flags to be always full.
author : Junhan
junhanLee95 added a commit to junhanLee95/rocksdb that referenced this pull request Jun 29, 2023
this fix is due to facebook#6104.
unscheduled_flushes_ was not handled well in the current version (v6.1.2.)
This leads flush_scheduled_ flags to be always full

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

Successfully merging this pull request may close these issues.

3 participants