Skip to content

fix overflowed parameter and add missed parameters#291

Merged
thweetkomputer merged 4 commits intoeloqdata:mainfrom
thweetkomputer:fix-overflow-zc
Nov 10, 2025
Merged

fix overflowed parameter and add missed parameters#291
thweetkomputer merged 4 commits intoeloqdata:mainfrom
thweetkomputer:fix-overflow-zc

Conversation

@thweetkomputer
Copy link
Collaborator

@thweetkomputer thweetkomputer commented Nov 5, 2025

Summary by CodeRabbit

  • Chores
    • Updated storage configuration options to accept human-readable size formats (e.g., "128MB") for improved usability.
    • Added support for new storage settings: skip verify checksum, data append mode, and enable compression.
    • Restructured internal storage configuration handling to better support flexible resource allocation.

@coderabbitai
Copy link

coderabbitai bot commented Nov 5, 2025

Warning

Rate limit exceeded

@thweetkomputer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 8 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 87ff48c and 41d093e.

📒 Files selected for processing (3)
  • src/mongo/db/modules/eloq/src/eloq_global_options.cpp (4 hunks)
  • src/mongo/db/modules/eloq/src/eloq_global_options.h (2 hunks)
  • src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1 hunks)

Walkthrough

The changes modify Eloq storage configuration by introducing new boolean fields for compression and verification skipping, converting index buffer pool size from integer to string-based storage, and adjusting resource sizing calculations across the global options and KV engine initialization logic.

Changes

Cohort / File(s) Summary
Configuration Header Updates
src/mongo/db/modules/eloq/src/eloq_global_options.h
Added three new public boolean fields: eloqStoreSkipVerifyChecksum, eloqStoreDataAppendMode, and eloqStoreEnableCompression. Converted eloqStoreIndexBufferPoolSize from uint32_t to std::string type. Reintroduced eloqStoreCloudStorePath, eloqStoreLocalSpaceLimit, and eloqStoreStoragePathList as string fields with adjusted field ordering.
Option Definition & Parsing
src/mongo/db/modules/eloq/src/eloq_global_options.cpp
Replaced integer-based index buffer pool size option with string-based variant (default "128MB" vs. previous 1 << 15). Added new public boolean option for eloqStoreEnableCompression with default false. Updated store() method to parse eloqStoreIndexBufferPoolSize as string and populate compression flag.
Engine Configuration
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
Modified EloqStoreConfig initialization to parse eloqStoreLocalSpaceLimit and eloqStoreIndexBufferPoolSize as strings via txlog::parse_size(). Changed resource sizing: local_space_limit now assigned directly; index_buffer_pool_size remains divided by thread count. Added new enable_compression flag assignment from global options.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Type conversion ripple: The eloqStoreIndexBufferPoolSize conversion from uint32_t to std::string requires verification that all downstream consumers (particularly in eloq_kv_engine.cpp) correctly invoke txlog::parse_size() and handle potential parsing errors.
  • Resource scaling logic change: The removal of per-thread division for local_space_limit could impact memory allocation behavior; verify this aligns with intended resource distribution strategy.
  • New configuration fields: Ensure eloqStoreEnableCompression and the skip-verify flags are properly threaded through the KV engine initialization and any dependent subsystems.
  • Cross-file consistency: Confirm that field declarations in the header match the assignments in both the .cpp files.

Suggested reviewers

  • xiexiaoy

Poem

🐰 Our config now strings where numbers once stood,
Compression unfurls, skipping what it should.
Fields dance and rearrange with newfound grace,
Parse sizes, flags spring—a storage-bound race! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title partially relates to the changeset by referencing a fix for overflow and added parameters, which matches some modifications present in the code. Consider making the title more specific about which parameters overflowed and which were added (e.g., 'Convert index buffer pool size to string and add compression options').
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

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

Copy link
Collaborator

@yi-xmu yi-xmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_global_options.h (1)

23-27: Remove duplicate include directives.

Lines 23-24 and 26-27 contain identical include directives for <cstdint> and <sys/types.h>.

Apply this diff to remove the duplicates:

 #include "mongo/util/options_parser/startup_option_init.h"
 #include "mongo/util/options_parser/startup_options.h"
 #include <cstdint>
 #include <sys/types.h>
-
-#include <cstdint>
-#include <sys/types.h>
 
 #include "mongo/db/modules/eloq/tx_service/include/cc_protocol.h"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f00dc2 and d01c4af.

📒 Files selected for processing (1)
  • src/mongo/db/modules/eloq/src/eloq_global_options.h (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🔇 Additional comments (2)
src/mongo/db/modules/eloq/src/eloq_global_options.h (2)

145-146: No issues found—both fields are properly integrated and utilized.

The verification confirms both eloqStoreCloudStorePath and eloqStoreStoragePathList are registered as command-line options, assigned from parameters, and actively used in the engine initialization. Each field is checked for non-empty status before use and passed to the storage configuration.


122-123: Both eloqStoreSkipVerifyChecksum and eloqStoreDataAppendMode are properly utilized throughout the codebase:

  • eloqStoreSkipVerifyChecksum is assigned to the engine config at eloq_kv_engine.cpp:198, registered as a command-line option at eloq_global_options.cpp:450-453, and parsed from parameters at eloq_global_options.cpp:988-990.

  • eloqStoreDataAppendMode is assigned to the engine config at eloq_kv_engine.cpp:249, registered as a command-line option at eloq_global_options.cpp:567-570, and parsed from parameters at eloq_global_options.cpp:1056-1058.

Both fields have appropriate default values and follow consistent integration patterns with proper parameter handling.

@thweetkomputer thweetkomputer marked this pull request as draft November 5, 2025 09:19
@thweetkomputer thweetkomputer force-pushed the fix-overflow-zc branch 2 times, most recently from c8655ab to 1ce95a9 Compare November 7, 2025 10:26
@thweetkomputer thweetkomputer marked this pull request as ready for review November 7, 2025 10:42
@thweetkomputer thweetkomputer changed the title fix overflowed parameter fix overflowed parameter and add missed parameters Nov 7, 2025
@thweetkomputer thweetkomputer requested a review from yi-xmu November 7, 2025 10:44
@thweetkomputer thweetkomputer merged commit 43bd744 into eloqdata:main Nov 10, 2025
3 checks passed
@thweetkomputer thweetkomputer deleted the fix-overflow-zc branch November 10, 2025 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants