Support Non-Blocking Manual Compactions (CompactRange) (#597) #656

udi-speedb · 2023-08-29T13:33:57Z

Add a new option to CompactRangeOptions, a callback object. By defaut, the callback is empty so CompactRange() will be blocking (as it is now). If the callback is set, the callback object's callback method will be called upon completion with the completion status.

ofriedma · 2023-08-29T15:28:28Z

@udi-speedb please add example file how to use the new non blocking compaction feature

ofriedma · 2023-08-29T15:51:57Z

please use port::Thread and not std::thread for spawning threads, so the on thread start callback will work on those threads

udi-speedb · 2023-08-30T04:45:44Z

please use port::Thread and not std::thread for spawning threads, so the on thread start callback will work on those threads

Done. Please just note that I have made the ThreadWithCb::joinable() a const method (same as std::thread::joinable()) (I am calling this method from a const method of mine).

udi-speedb · 2023-08-30T08:31:40Z

@udi-speedb please add example file how to use the new non blocking compaction feature

Done

ofriedma · 2023-08-30T14:19:04Z

@udi-speedb add a unit test for closing the db before compact range has been completed

udi-speedb · 2023-08-31T08:53:29Z

@udi-speedb add a unit test for closing the db before compact range has been completed

As we have discussed, it will be the user's responsibility to wait for all non-blocking CompactRange() calls to have their respective callbacks called. Thereafter, the Close() shouldn't have any issues joining the internal threads.
I will add a note about that in the CompactRange()'s documentation.

ofriedma · 2023-09-04T08:53:30Z

db/db_compaction_test.cc

+using ValidateCompletionStatusFunc =
+    std::function<void(ROCKSDB_NAMESPACE::Status completion_status)>;
+
+void DefaultCompletionStatusValidation(Status completion_status,


above this line you added the ROCKSDB_NAMESPACE:: , please use it here or remove it from the other places.

I have removed in new code that I have added, however, there are many places in the db_compaction_test.cc that use "ROCKSDB_NAMESPACE::" which I will not touch to avoid unnecessary code changes that are irrelevant to this PR.

ofriedma · 2023-09-04T08:56:26Z

db/db_compaction_test.cc

@@ -134,32 +386,70 @@ class DBCompactionTestWithParam
  bool exclusive_manual_compaction_;
 };

+class DBCompactionTestWithParamWithMCC


What is MCC?

Manual Compaction Control

ofriedma · 2023-09-04T09:14:18Z

db/db_impl/compact_range_threads_mngr.cc

+  threads_.push_back(std::move(thread));
+}
+
+void CompactRangeThreadsMngr::CleanupCompletedThreads() {


I think joinable() will always return true, because we have never called join().

ofriedma · 2023-09-04T09:21:45Z

db/db_impl/db_impl.cc

@@ -622,6 +622,10 @@ Status DBImpl::CloseHelper() {
    cfd->UnrefAndTryDelete();
  }

+  // Wait for all non-blocking manual compactions that may still be in progress.
+  // Do it only after cleaning up all compaction-related activity above.
+  compact_range_threads_mngr_.Shutdown();


ofriedma · 2023-09-04T09:23:33Z

db/db_impl/db_impl.h

@@ -2756,6 +2773,8 @@ class DBImpl : public DB {
  // The number of LockWAL called without matching UnlockWAL call.
  // See also lock_wal_write_token_
  uint32_t lock_wal_count_;
+


maybe add a small comment here about the manual/range compaction thread manager

please notice

ofriedma · 2023-09-04T09:31:48Z

db_stress_tool/db_stress_test_base.cc

+    } else {
+      fprintf(stderr,
+              "Non-Blocking CompactRange() Didn't Complete Successfuly in "
+              "Time: %d\n",


Maybe print the status message?

This is std::future::future_status which is an enum, what status message should I print?

ofriedma · 2023-09-04T09:34:18Z

include/rocksdb/options.h

+
+  // An optional completion callback to allow for non-blocking (async) operation
+  // Default: Empty (Blocking)
+  std::shared_ptr<CompactRangeCompletedCbIf> async_completion_cb;


async_completion_cb, maybe use another name, what if we will want to add Async PUT for example?

ofriedma · 2023-09-04T09:34:31Z

include/rocksdb/port_defs.h

@@ -36,7 +36,7 @@ class ThreadWithCb {
  }

  ThreadWithCb() {}
-  bool joinable() { return thread_.joinable(); }
+  bool joinable() const { return thread_.joinable(); }


ofriedma · 2023-09-04T09:39:59Z

java/src/test/java/org/rocksdb/CompactRangeCompletedCbTest.java

Add license here

I have removed this file

ofriedma

before merge please add comments

ofriedma · 2023-09-05T12:06:18Z

db/db_compaction_test.cc

+
+}  // namespace
+
+class DBCompactionTestWithMCC : public DBCompactionTest,


Please add a note what is MCC.

ofriedma · 2023-09-05T12:14:42Z

db/db_impl/db_impl.h

@@ -2756,6 +2773,8 @@ class DBImpl : public DB {
  // The number of LockWAL called without matching UnlockWAL call.
  // See also lock_wal_write_token_
  uint32_t lock_wal_count_;
+


please notice

Add a new option to CompactRangeOptions, a callback object. By defaut, the callback is empty so CompactRange() will be blocking (as it is now). If the callback is set, the callback object's callback method will be called upon completion with the completion status.

Add a new option to CompactRangeOptions, a callback object. By default, the callback is empty so CompactRange() will be blocking (as it is now). If the callback is set, the callback object's callback method will be called upon completion with the completion status.

udi-speedb requested a review from ofriedma August 29, 2023 13:33

udi-speedb linked an issue Aug 29, 2023 that may be closed by this pull request

Make compactRange() non-blocking #597

Closed

udi-speedb force-pushed the 597-make-compactrange-non-blocking branch from ad48fb2 to 9a686a1 Compare August 30, 2023 05:12

udi-speedb force-pushed the 597-make-compactrange-non-blocking branch from 980510e to 2c93929 Compare August 30, 2023 08:57

ofriedma requested changes Sep 4, 2023

View reviewed changes

ofriedma added the enhancement New feature or request label Sep 4, 2023

ofriedma reviewed Sep 4, 2023

View reviewed changes

ofriedma approved these changes Sep 5, 2023

View reviewed changes

udi-speedb requested a review from ofriedma September 5, 2023 14:23

ofriedma approved these changes Sep 6, 2023

View reviewed changes

udi-speedb force-pushed the 597-make-compactrange-non-blocking branch from 129e3d6 to 3cf02ac Compare September 6, 2023 12:23

Yuval-Ariel merged commit ea3da8e into main Sep 18, 2023
2 checks passed

Yuval-Ariel deleted the 597-make-compactrange-non-blocking branch September 18, 2023 12:22

Yuval-Ariel mentioned this pull request Sep 28, 2023

unit tests: some tests are stuck #697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Non-Blocking Manual Compactions (CompactRange) (#597) #656

Support Non-Blocking Manual Compactions (CompactRange) (#597) #656

udi-speedb commented Aug 29, 2023

ofriedma commented Aug 29, 2023

ofriedma commented Aug 29, 2023

udi-speedb commented Aug 30, 2023

udi-speedb commented Aug 30, 2023

ofriedma commented Aug 30, 2023

udi-speedb commented Aug 31, 2023

ofriedma Sep 4, 2023

udi-speedb Sep 4, 2023

ofriedma Sep 4, 2023

udi-speedb Sep 4, 2023

ofriedma Sep 4, 2023

ofriedma Sep 4, 2023

ofriedma Sep 4, 2023

ofriedma Sep 5, 2023

ofriedma Sep 4, 2023

udi-speedb Sep 4, 2023

ofriedma Sep 4, 2023

ofriedma Sep 4, 2023

ofriedma Sep 4, 2023

udi-speedb Sep 5, 2023

ofriedma left a comment

ofriedma Sep 5, 2023

udi-speedb Sep 5, 2023

ofriedma Sep 5, 2023


		} // namespace

		class DBCompactionTestWithMCC : public DBCompactionTest,

Support Non-Blocking Manual Compactions (CompactRange) (#597) #656

Support Non-Blocking Manual Compactions (CompactRange) (#597) #656

Conversation

udi-speedb commented Aug 29, 2023

ofriedma commented Aug 29, 2023

ofriedma commented Aug 29, 2023

udi-speedb commented Aug 30, 2023

udi-speedb commented Aug 30, 2023

ofriedma commented Aug 30, 2023

udi-speedb commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofriedma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment