Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Google Logging initialization and header inclusion in EloqKVEngine and updates the eloq data_substrate submodule pointer; no changes to exported declarations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes the log directory configuration for the Eloq storage engine by properly initializing Google logging with the system's configured log path. The changes ensure that glog writes to the correct directory based on MongoDB's server configuration.
Key Changes:
- Added initialization of Google logging (glog) with proper log directory configuration extracted from MongoDB's server global parameters
- Updated the data_substrate submodule to a newer version
- Reformatted Python code in SConscript for improved readability and style consistency
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp | Added glog initialization with log directory configuration from server parameters |
| src/mongo/db/modules/eloq/data_substrate | Updated submodule commit reference |
| src/mongo/db/modules/eloq/SConscript | Applied Python code formatting improvements and added jemalloc support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| std::filesystem::path systemLogPath(serverGlobalParams.logpath); | ||
| if (systemLogPath.has_parent_path()) { | ||
| static std::filesystem::path logdir = systemLogPath.parent_path(); |
There was a problem hiding this comment.
The variable is declared as static, which means it will persist across multiple constructor invocations. If the EloqKVEngine constructor is called multiple times with different paths, the logdir will retain the value from the first invocation and ignore subsequent changes to serverGlobalParams.logpath. Consider removing the static keyword unless there's a specific reason to persist this value across instances.
| static std::filesystem::path logdir = systemLogPath.parent_path(); | |
| std::filesystem::path logdir = systemLogPath.parent_path(); |
| char** dummy_argv = const_cast<char**>(tmp); | ||
| InitGoogleLogging(dummy_argv); |
There was a problem hiding this comment.
The function InitGoogleLogging typically expects a single char* argument (the program name), but this code passes char**. Verify that this is the correct API signature for the version of glog being used. Most glog versions use InitGoogleLogging(const char* argv0) which would only require InitGoogleLogging("eloqdb").
| char** dummy_argv = const_cast<char**>(tmp); | |
| InitGoogleLogging(dummy_argv); | |
| // char** dummy_argv = const_cast<char**>(tmp); | |
| InitGoogleLogging(tmp[0]); |
| if jemalloc_lib is None: | ||
| print("ERROR: WITH_JEMALLOC=ON but libjemalloc not found") | ||
| Exit(1) | ||
| jemalloc_deps = [] |
There was a problem hiding this comment.
The variable jemalloc_deps is initialized to an empty list twice: once before the conditional block at line 143 and again inside the conditional block at line 150. Remove the redundant initialization at line 150.
| jemalloc_deps = [] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mongo/db/modules/eloq/SConscript (1)
122-138: Bug:LOG_STATE_TYPE_RKDB_MEMORYis undefined.Line 124 references
LOG_STATE_TYPE_RKDB_MEMORY, but onlyLOG_STATE_TYPE_MEMis defined at line 66. This will cause aNameErrorat runtime whenWITH_LOG_STATE=MEMORYis set.if with_log_state_str == "MEMORY": - with_log_state = LOG_STATE_TYPE_RKDB_MEMORY + with_log_state = LOG_STATE_TYPE_MEM env.Append(CPPDEFINES=["LOG_STATE_TYPE_MEM"])
🧹 Nitpick comments (4)
src/mongo/db/modules/eloq/SConscript (4)
143-151: Redundant re-initialization ofjemalloc_deps.Line 150 re-initializes
jemalloc_deps = []immediately before appending, but it was already initialized at line 143. This is harmless but redundant.jemalloc_deps = [] if os.environ.get("WITH_JEMALLOC", "0") in ("1", "ON", "on", "true", "True"): env.Append(CPPDEFINES=["WITH_JEMALLOC"]) jemalloc_lib = FindLibPath("jemalloc") if jemalloc_lib is None: print("ERROR: WITH_JEMALLOC=ON but libjemalloc not found") Exit(1) - jemalloc_deps = [] jemalloc_deps.append(jemalloc_lib)
44-44: Consider removing debug print statement.This debug print will produce output for every candidate path checked during the build, which could be noisy. Consider removing or making it conditional on a verbose flag.
- print("candidate:", candidate)
171-199: Potential duplicate include paths.The include paths for
rocksdb_cloud_header,aws/*, andgoogle/cloud/storageare added unconditionally at lines 171-178 and then conditionally added again at lines 184-199. While SCons may deduplicate these, it's cleaner to add them only once. Consider removing the unconditional additions at lines 172-178 since they're properly handled conditionally below.Additionally, lines 171-178 don't filter out
Nonevalues fromFindIncludePath, which could addNonetoCPPPATH.
305-327: Duplicate library entries in GCS log state branch.Lines 306-307 add
google_cloud_cpp_common,google_cloud_cpp_storage, androcksdb-cloud-gcp, then lines 308-312 add them again within the same conditional block. This duplication is likely unintentional.elif with_log_state == LOG_STATE_TYPE_RKDB_GCS: env.Append(LIBS=["google_cloud_cpp_common"]) env.Append(LIBS=["google_cloud_cpp_storage", "rocksdb-cloud-gcp"]) env.Append(LIBS=[ - "google_cloud_cpp_common", - "google_cloud_cpp_storage", - "rocksdb-cloud-gcp", "rocksdb", "absl_base", ... ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mongo/db/modules/eloq/SConscript(13 hunks)src/mongo/db/modules/eloq/data_substrate(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
📚 Learning: 2025-11-28T10:55:21.020Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/modules/eloq/CMakeLists.txt:54-55
Timestamp: 2025-11-28T10:55:21.020Z
Learning: In src/mongo/db/modules/eloq/CMakeLists.txt, the CMake options EXT_TX_PROC_ENABLED, FORK_HM_PROCESS, and STATISTICS are used by the data_substrate submodule build system and should be retained as configuration interface options.
Applied to files:
src/mongo/db/modules/eloq/data_substratesrc/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 Learning: 2025-09-25T11:58:50.446Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_log_service.cmake:116-119
Timestamp: 2025-09-25T11:58:50.446Z
Learning: The build_log_service.cmake file is specifically for "open log" functionality and only supports the ROCKSDB log state type (LOG_STATE_TYPE_RKDB). The full log state configuration with cloud variants (ROCKSDB_CLOUD_S3, ROCKSDB_CLOUD_GCS) is handled in build_eloq_log_service.cmake.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 Learning: 2025-09-25T12:09:01.276Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:95-115
Timestamp: 2025-09-25T12:09:01.276Z
Learning: The log states (LOG_STATE_TYPE_RKDB, LOG_STATE_TYPE_RKDB_S3, LOG_STATE_TYPE_RKDB_GCS) in the EloqDoc codebase are mutually exclusive - only one can be active at a time. The LOG_STATE_TYPE_RKDB_CLOUD macro is a helper that should only be defined for cloud variants (S3/GCS) and not when regular RKDB is active.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/SConscript
📚 Learning: 2025-11-28T10:52:25.527Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: concourse/scripts/main.ent.bash:38-39
Timestamp: 2025-11-28T10:52:25.527Z
Learning: In the eloqdoc enterprise build (main.ent.bash), eloq_log_service is fetched separately as the resource `eloq_logservice_src` in the pipeline (main.ent.yml) and then symlinked into the data_substrate directory structure at build time using `ln -s $WORKSPACE/eloq_logservice_src eloq_log_service`. This is the intended architecture for placing eloq_log_service within data_substrate in the enterprise configuration.
Applied to files:
src/mongo/db/modules/eloq/SConscript
🪛 Cppcheck (2.18.0)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
[error] 161-161: #error "Unsupported compiler family"
(preprocessorErrorDirective)
🪛 GitHub Actions: Copilot code review
src/mongo/db/modules/eloq/data_substrate
[error] 1-1: autofind failed: Error getting common flags: diff references file 'src/mongo/db/modules/eloq/data_substrate' that is not present in repo (likely incorrect repo root).
🔇 Additional comments (3)
src/mongo/db/modules/eloq/data_substrate (1)
1-1: Verify the submodule commit aligns with PR objectives.This is a submodule update to
data_substrate. Without visibility into what changed in the submodule, please verify that the new commit (834641d...) relates to the PR objective ("fix log dir") and does not introduce breaking changes or require additional coordination with the related Google Logging initialization and build configuration changes mentioned in the summary.To help verify this, please provide:
- A summary of what changed in the
data_substratesubmodule between the old and new commits- Confirmation that the update is compatible with the Google Logging initialization changes to
eloq_kv_engineand the build configuration refinements mentioned in the PR summarysrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
38-38: LGTM!The include for
glog_error_logging.his correctly added to support the new Google Logging initialization.
153-162: Glog initialization is safe in this codebase despite the general concern.glog does not support multiple calls to InitGoogleLogging and will produce a fatal error if called twice. However, the proposed guard is unnecessary here because EloqKVEngine is instantiated exactly once in the factory pattern (eloq_init.cpp:48 via
make_unique), and the class has deleted copy/move constructors preventing accidental duplication.The real concern is the secondary issue: add a default or warning when
serverGlobalParams.logpathhas no parent directory, as log_dir will silently remain unset in that case.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.