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

set purges as Pri::LOW instead of HIGH #34

Closed
Yuval-Ariel opened this issue Jun 28, 2022 · 5 comments · Fixed by #151
Closed

set purges as Pri::LOW instead of HIGH #34

Yuval-Ariel opened this issue Jun 28, 2022 · 5 comments · Fixed by #151
Assignees
Milestone

Comments

@Yuval-Ariel
Copy link
Contributor

reason for the change:

purges are deletions of obsolete compaction files. by doing the cleanup of these files in the HIGH priority thread pool, the flushes are hindered. we’d like to avoid disturbing the flushes and instead do this cleaning job as a LOW priority.

other notes:

this change is also favored when using the SPDB-680 (delete a file in chunks) feature since with it, the deletion takes longer which would further stall the flushes.

also, consider changing priority here as well db/db_impl/db_impl_compaction_flush.cc:

void DBImpl::BGWorkPurge(void* db) {
  IOSTATS_SET_THREAD_POOL_ID(Env::Priority::HIGH);
  TEST_SYNC_POINT("DBImpl::BGWorkPurge:start");
  reinterpret_cast<DBImpl*>(db)->BackgroundCallPurge();
  TEST_SYNC_POINT("DBImpl::BGWorkPurge:end");
}
Yuval-Ariel added a commit that referenced this issue Jun 28, 2022
@isaac-io isaac-io linked a pull request Jul 26, 2022 that will close this issue
AmnonHanuhov pushed a commit that referenced this issue Sep 6, 2022
@AmnonHanuhov AmnonHanuhov linked a pull request Sep 6, 2022 that will close this issue
@isaac-io isaac-io added this to the v2.1.0 milestone Sep 21, 2022
@assaf-speedb
Copy link
Contributor

Unit tests passed.

@isaac-io
Copy link
Contributor

@erez-speedb this has been accidentally merged to main without running the performance tests. Please run them on commit d3a10864f41bec34f412b8203821d86aca366921 in main and compare the before and after with avoid_unnecessary_blocking_io after #183 is merged (I'll ping you when that happens).

@isaac-io
Copy link
Contributor

#183 has been merged. @erez-speedb, please run the aforementioned test on and before commit d3a10864f41bec34f412b8203821d86aca366921 as specified in my previous message.

isaac-io pushed a commit that referenced this issue Oct 19, 2022
isaac-io pushed a commit that referenced this issue Oct 19, 2022
isaac-io pushed a commit that referenced this issue Oct 19, 2022
isaac-io pushed a commit that referenced this issue Oct 19, 2022
isaac-io pushed a commit that referenced this issue Oct 19, 2022
@Yuval-Ariel
Copy link
Contributor Author

@isaac-io we should also change the description here right?

include/rocksdb/options.h
// If true, when PurgeObsoleteFile is called in CleanupIteratorState, we
// schedule a background job in the flush job queue and delete obsolete files
// in background.
// Default: false
bool background_purge_on_iterator_cleanup;

@isaac-io
Copy link
Contributor

Yes. Would you be able to open a PR to fix this? I don't think that this requires an accompanying issue.

Yuval-Ariel added a commit that referenced this issue Oct 20, 2022
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
Yuval-Ariel added a commit that referenced this issue Oct 20, 2022
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
isaac-io pushed a commit that referenced this issue Oct 20, 2022
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
isaac-io pushed a commit that referenced this issue Oct 20, 2022
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
isaac-io pushed a commit that referenced this issue Oct 24, 2022
isaac-io pushed a commit that referenced this issue Oct 24, 2022
Yuval-Ariel added a commit that referenced this issue Nov 23, 2022
Yuval-Ariel added a commit that referenced this issue Nov 23, 2022
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
Yuval-Ariel added a commit that referenced this issue Nov 25, 2022
Yuval-Ariel added a commit that referenced this issue Nov 25, 2022
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
Yuval-Ariel added a commit that referenced this issue Apr 30, 2023
Yuval-Ariel added a commit that referenced this issue Apr 30, 2023
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
Yuval-Ariel added a commit that referenced this issue May 4, 2023
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
udi-speedb pushed a commit that referenced this issue Oct 31, 2023
udi-speedb pushed a commit that referenced this issue Nov 13, 2023
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
udi-speedb pushed a commit that referenced this issue Dec 3, 2023
udi-speedb pushed a commit that referenced this issue Dec 3, 2023
since purges are now set from the LOW pri pool,
the bg job scheduled when background_purge_on_iterator_cleanup is
true is scheduled in the compaction queue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants