should we start the compaction_checker_range thread if it's not configed.

13015517713 commented 3 months ago

Search before asking

[X] I had searched in the issues and found no similar issues.

Motivation

When running Server.start(), kvrocks will launch a thread for compaction check. Some considerations are as follows.

If there is no checker configured, maybe it is no need to start the thread to check frequently. Only if it is enabled, then create the thread.
The checker for pubsubfiles is independent of the configuration of the normal checker (the code is like this). If there is no configured checker, kvrocks also should do compression for pubsub. (not sure I understand that correctly.)

Corresponding code block is in server.cc, Server().Start().

compaction_checker_thread_ = GET_OR_RET(util::CreateThread("compact-check", [this] {
    uint64_t counter = 0;
    int64_t last_compact_date = 0;
    CompactionChecker compaction_checker{this->storage};

    while (!stop_) {
      // Sleep first
      std::this_thread::sleep_for(std::chrono::milliseconds(100));

      // To guarantee accessing DB safely
      auto guard = storage->ReadLockGuard();
      if (storage->IsClosing()) continue;

      if (!is_loading_ && ++counter % 600 == 0  // check every minute
          && config_->compaction_checker_range.Enabled()) {
        auto now_hours = util::GetTimeStamp<std::chrono::hours>();
        if (now_hours >= config_->compaction_checker_range.start &&
            now_hours <= config_->compaction_checker_range.stop) {
          std::vector<std::string> cf_names = {engine::kMetadataColumnFamilyName, engine::kSubkeyColumnFamilyName,
                                               engine::kZSetScoreColumnFamilyName, engine::kStreamColumnFamilyName};
          for (const auto &cf_name : cf_names) {
            compaction_checker.PickCompactionFiles(cf_name);
          }
        }
        // compact once per day
        if (now_hours != 0 && last_compact_date != now_hours / 24) {
          last_compact_date = now_hours / 24;
          compaction_checker.CompactPropagateAndPubSubFiles();
        }
      }
    }
  }));

Solution

No response

Are you willing to submit a PR?

[ ] I'm willing to submit a PR!

git-hulk commented 3 months ago

@13015517713 Thanks for bringing up this discussion. It's unnecessary to check if disabled since the compaction will be totally depended on the rocksdb.

mapleFU commented 3 months ago

The range isn't read-only, and the thread is not so high-cost. I think it's ok to not dynamically enable it. But if we can have a timer task system, it would also be ok to execute the check in the timer rather than a separate thread

git-hulk commented 3 months ago

Yes, it's good to leave as it is to keep the codes simple.

apache / kvrocks