Remove USE_ROCKSDB_LOG_STATE and WITH_ROCKSDB_CLOUD compile flag

[githubzilla]

Here are some reminders before you submit the pull request

• Add tests for the change
• Document changes
• Reference the link of issue using fixes eloqdb/tx_service#issue_id
• Reference the link of RFC if exists
• Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

• Refactor

  • Replaced multiple build flags with a single WITH_LOG_STATE option to select the log-state backend (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS).
  • Unified compile-time definitions per selection, added a fatal error for invalid values, and emit the chosen option during configuration.
  • Applied the same streamlining to test builds.

• Chores

  • Bumped minimum CMake requirement and removed deprecated flags and conditional logic.

[coderabbitai]

Warning

Rate limit exceeded

@githubzilla has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 17 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between acf638f and b67d3c5.

📒 Files selected for processing (2)

• CMakeLists.txt (2 hunks)
• tests/CMakeLists.txt (1 hunks)

Walkthrough

Consolidated log-state selection into a single cached CMake variable WITH_LOG_STATE (values: MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS), added corresponding compile-time definitions, printed the selection, added a fatal error for unknown values, and bumped required CMake versions.

Changes

Cohort / File(s)	Summary
Root build configuration CMakeLists.txt	Added WITH_LOG_STATE (STRING) with STRINGS property (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS); set compile defs per value (LOG_STATE_TYPE_MEM, LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS); printed WITH_LOG_STATE; fatal error on unknown value; removed USE_ROCKSDB_LOG_STATE / WITH_ROCKSDB_CLOUD logic; updated cmake_minimum_required 3.8 → 3.12.
Test build configuration tests/CMakeLists.txt	Mirrored WITH_LOG_STATE cache variable and STRINGS property; applied same compile defs and unknown-value fatal error; replaced prior USE_ROCKSDB_LOG_STATE / WITH_ROCKSDB_CLOUD branching with explicit per-value branches; updated cmake_minimum_required 3.0 → 3.12; preserved test targets.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Cfg as CMake (configure)
    participant Cache as WITH_LOG_STATE
    participant Compile as add_compile_definitions

    Cfg->>Cache: read cached WITH_LOG_STATE
    alt MEMORY
        Cache-->>Compile: LOG_STATE_TYPE_MEM
    else ROCKSDB
        Cache-->>Compile: LOG_STATE_TYPE_RKDB
    else ROCKSDB_CLOUD_S3
        Cache-->>Compile: LOG_STATE_TYPE_RKDB_S3
    else ROCKSDB_CLOUD_GCS
        Cache-->>Compile: LOG_STATE_TYPE_RKDB_GCS
    else Unknown
        Cache-->>Cfg: message(FATAL_ERROR "Unknown WITH_LOG_STATE: ...")
    end
    Note over Compile: Definitions applied at configure time\nand visible to root and tests

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my nose at options few,
One flag now guides the build-through.
MEMORY burrows, ROCKSDB runs,
S3 and GCS catch cloud-side suns.
Stray and CMake gives a thump— I hop delighted—no more grump! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description contains only the repository reminder checklist template and does not include a summary of the actual changes, motivation, links to issues or RFCs, test results, or documentation notes, so it is incomplete and insufficient for review.	Update the PR description to include a concise summary of what changed and why, list any related issue or RFC links, describe documentation updates or migration notes, and provide tests added or test run output (e.g., ./mtr results); mark the checklist items as completed after adding this information.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title accurately and concisely describes the primary change — removal of the USE_ROCKSDB_LOG_STATE and WITH_ROCKSDB_CLOUD compile flags — which matches the CMake edits that replace them with the new WITH_LOG_STATE variable and related logic, and it is specific enough for a reviewer to understand the main intent.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

CMakeLists.txt (1)

28-39: Prefer target-scoped definitions (follow-up).

Longer term, move these to target_compile_definitions for txservice, host_manager, and test targets to avoid global pollution. Create a small INTERFACE lib (e.g., log_state_config) carrying these defines and link it where needed.

tests/CMakeLists.txt (1)

8-23: Avoid duplication: centralize the WITH_LOG_STATE-to-macros mapping.

Factor the selection logic into a shared CMake module (e.g., cmake/LogStateConfig.cmake) and include() it in both root and tests to keep behavior in sync.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bd068a and 7545f2f.

📒 Files selected for processing (2)

• CMakeLists.txt (1 hunks)
• tests/CMakeLists.txt (1 hunks)

🔇 Additional comments (4)

CMakeLists.txt (2)

24-26: Good: single source of truth for log state selection.

WITH_LOG_STATE with constrained values and a NOTICE print is a clear improvement over the legacy flags.

---

24-39: Confirm legacy flags removed and that LOG_STATE_TYPE_ are actually consumed*

• Search found no occurrences of USE_ROCKSDB_LOG_STATE or WITH_ROCKSDB_CLOUD.
• LOG_STATE_TYPE_* are defined via add_compile_definitions only in CMakeLists.txt and tests/CMakeLists.txt; no source-level references were found — confirm code paths check/use these macros or that the build correctly propagates them.

tests/CMakeLists.txt (2)

8-11: LGTM on consolidating WITH_LOG_STATE in tests too.

Keeping the allowed values aligned with the root is good for consistency.

---

8-23: Sanity-check cloud variants build.

If ROCKSDB_CLOUD_S3/GCS require extra deps, add corresponding find_package/flags where needed or conditionally gate tests to avoid failing CI when those SDKs are absent.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

CMakeLists.txt (1)

38-38: Improve fatal error message with allowed values.

Helps users immediately see valid options.

Apply this diff:

-  message(FATAL_ERROR "Unknown WITH_LOG_STATE: ${WITH_LOG_STATE}")
+  message(FATAL_ERROR "Unknown WITH_LOG_STATE: ${WITH_LOG_STATE} (allowed: MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS)")

tests/CMakeLists.txt (1)

7-19: Avoid duplication of the LOG_STATE mapping between root and tests.

Factor the mapping into a shared CMake include to prevent drift.

Example approach:

• Create cmake/LogState.cmake:

# cmake/LogState.cmake
if(WITH_LOG_STATE STREQUAL "MEMORY")
  add_compile_definitions(LOG_STATE_TYPE_MEM)
elseif(WITH_LOG_STATE STREQUAL "ROCKSDB")
  add_compile_definitions(LOG_STATE_TYPE_RKDB)
elseif(WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_S3")
  add_compile_definitions(LOG_STATE_TYPE_RKDB_S3)
elseif(WITH_LOG_STATE STREQUAL "ROCKSDB_CLOUD_GCS")
  add_compile_definitions(LOG_STATE_TYPE_RKDB_GCS)
else()
  message(FATAL_ERROR "Unknown WITH_LOG_STATE: ${WITH_LOG_STATE} (allowed: MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS)")
endif()

• Then replace the duplicated blocks with:

include(${CMAKE_SOURCE_DIR}/cmake/LogState.cmake)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7545f2f and 0285b88.

📒 Files selected for processing (2)

• CMakeLists.txt (2 hunks)
• tests/CMakeLists.txt (1 hunks)

🔇 Additional comments (5)

CMakeLists.txt (3)

1-1: Min version bump is appropriate for add_compile_definitions.

Setting CMake >= 3.12 aligns with add_compile_definitions usage.

---

28-39: Earlier concern resolved by raising min CMake.

Using add_compile_definitions is fine with VERSION 3.12+.

---

24-25: No stale flags found — WITH_LOG_STATE replaces the removed flags

USE_ROCKSDB_LOG_STATE and WITH_ROCKSDB_CLOUD are absent. CMakeLists.txt and tests/CMakeLists.txt define WITH_LOG_STATE (MEMORY, ROCKSDB, ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) and add LOG_STATE_TYPE_{MEM,RKDB,RKDB_S3,RKDB_GCS); remaining related macro: src/store/snapshot_manager.cpp:449-450 (ROCKSDB_CLOUD_FS_TYPE).

tests/CMakeLists.txt (2)

1-1: Min version aligns with add_compile_definitions, but see NOTICE usage below.

If keeping 3.12, adjust message level accordingly.

---

11-21: Earlier CMake compatibility issue appears resolved.

add_compile_definitions is safe now that min is 3.12.