Conversation
|
Warning Rate limit exceeded@liunyl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (4)
WalkthroughIntroduce a two-phase startup: initialize DataSubstrate instance, enable and start substrate engines, then call a new RedisServiceImpl::Start(brpc::Server&) to attach TxService, DataStoreHandler, metrics, slow-log, and vector components after substrate start. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Startup script
participant App as redis_server (main)
participant DS as DataSubstrate::Instance
participant RS as RedisServiceImpl
participant BRPC as brpc::Server
participant Tx as TxService / StoreHandler
Admin->>App: launch with config
App->>DS: Init(config_file)
DS-->>App: initialized
App->>DS: EnableEngine(EloqKv)
DS-->>App: engine enabled
App->>DS: Start()
DS-->>App: substrate running
App->>RS: construct RedisServiceImpl (unique_ptr)
App->>RS: Start(brpc::Server&)
RS->>DS: ds.RegisterEngine / Attach TxService & StoreHandler
DS-->>RS: TxService & StoreHandler available
RS->>RS: init prebuilt tables, metrics, slow-log, vector handler
RS-->>BRPC: register service (via options)
BRPC-->>Admin: server ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
include/redis_service.h (1)
168-170: Clarify Start() lifecycle expectations in class-level docsThe second-phase
Start(brpc::Server &)fits the new init flow. Consider adding a brief class comment (or doxygen onStart) stating it must be called exactly once, afterDataSubstrate::Start()and beforebrpc::Server::Start(), and is not thread-safe, to avoid misuse by future callers.src/redis_server.cpp (1)
452-501: VerifyEnableEnginevsRegisterEngineorderingThe new startup sequence is clear, but note that
DataSubstrate::Instance().EnableEngine(TableEngine::EloqKv)is called beforeRedisServiceImpl::Init(), which in turn callsDataSubstrate::Instance().RegisterEngine(...). Ifdata_substrateexpects engines to be registered before being enabled, this ordering could be problematic.If the library contract prefers “register → enable”, consider moving
EnableEngineafter a successfulRedisServiceImpl::Init()(or moving it into Init afterRegisterEngine), so the sequence is:
DataSubstrate::Instance().Init(...)RedisServiceImpl::Init(...)(doesRegisterEngine)DataSubstrate::Instance().EnableEngine(...)DataSubstrate::Instance().Start()src/redis_service.cpp (2)
446-569: Harden slow-log ID initialization and vector sizing in Start()The refactored
Start()correctly defers DS-dependent work (TxService/DataStore attach, slow-log, metrics, vector handler) until afterDataSubstrate::Start(). Two nits around slow-log state, though:
next_slow_log_unique_id_is declared asstd::vector<uint32_t>but seeded as if it were 64‑bit:// high 16 bits: core_id // low 48 bits: counter next_slow_log_unique_id_.push_back(core_idx << 48);Storing
(core_idx << 48)into auint32_tsilently truncates the high bits, so the “high 16 bits = core_id” invariant never actually holds. If you care about that layout, consider changing the member tostd::vector<uint64_t>and initializing viaresize/index assignment, e.g.:- std::vector<uint32_t> next_slow_log_unique_id_; + std::vector<uint64_t> next_slow_log_unique_id_; ... - next_slow_log_unique_id_.push_back(core_idx << 48); + next_slow_log_unique_id_[core_idx] = (uint64_t(core_idx) << 48);
- Unlike the other slow‑log vectors,
next_slow_log_unique_id_is not cleared/resized; you onlypush_backper core. IfStart()were ever called more than once (orcore_num_changed), this vector would keep growing and drift fromcore_num_. For robustness and symmetry withslow_log_,next_slow_log_idx_, andslow_log_len_, it would be safer toclear()orresize(core_num_)before seeding values.Neither issue breaks the current single‑start flow, but tightening this makes the slow‑log bookkeeping match its documented 64‑bit ID semantics and avoids surprises if the lifecycle ever changes.
551-566: Vector handler Start() path looks correct, but depends on DS & config parseThe
#ifdef VECTOR_INDEX_ENABLEDblock that initializesEloqVec::VectorHandlerinStart()(aftertx_service_is available) is a good move and lines up with theInitHandlerInstancesignature.Since you re-parse
config_file_here just for vector configuration, you pay the INI parse cost twice (once inInit, once here). Not a big deal, but if startup parsing ever becomes a bottleneck, you could consider sharing a parsed config or extracting just the minimal vector config needed into a struct and passing that instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data_substrate(1 hunks)include/redis_service.h(1 hunks)src/redis_server.cpp(4 hunks)src/redis_service.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
include/redis_service.h (1)
src/redis_service.cpp (2)
Start(446-570)Start(446-446)
src/redis_service.cpp (2)
src/vector/vector_handler.cpp (4)
Instance(125-132)Instance(125-125)InitHandlerInstance(92-118)InitHandlerInstance(92-96)src/redis_server.cpp (1)
config_reader(441-441)
🔇 Additional comments (2)
src/redis_service.cpp (2)
186-259: Engine registration in Init() looks consistent with prebuilt tables & metricsThe new block that:
- builds
prebuilt_tablesfordatabasesRedis DBs,- defines
engine_metricsfromEloqKV::command_types, and- calls
DataSubstrate::Instance().RegisterEngine(TableEngine::EloqKv, ...)is coherent with the rest of the class state (
redis_table_names_, metrics names) and keeps Redis-specific engine wiring localized inRedisServiceImpl::Init().Given only one
RedisServiceImplis constructed inmain(), repeated registration is not a concern in the current flow.
6051-6055: MaxConnectionCount() now depends on DataSubstrate core configSwitching
MaxConnectionCount()to:auto &ds = DataSubstrate::Instance(); return ds.GetCoreConfig().maxclients;is consistent with the broader DS‑driven configuration approach and should keep connection limits aligned with DS settings, as long as this is only called after
DataSubstrate::Init()(which main() currently guarantees).No change requested; just noting the dependency so future call sites don’t invoke this before DS initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/redis_server.cpp (1)
534-535: CRITICAL: Reverse shutdown order to prevent use-after-free.The current shutdown order calls
DataSubstrate::Instance().Shutdown()(line 534) beforeredis_service_ptr->Stop()(line 535). This is incorrect because:
- The metrics collector thread is stopped and joined in
RedisServiceImpl::Stop()- This thread accesses
metrics::redis_meter, which is initialized and managed by DataSubstrate (as noted inStart())- Tearing down DataSubstrate first can cause the metrics thread to access freed/invalid metrics state
This matches the concern raised in the past review comment. All error paths in this file correctly call
redis_service_ptr->Stop()beforeDataSubstrate::Instance().Shutdown(). The normal shutdown path should follow the same pattern.Apply this fix:
- DataSubstrate::Instance().Shutdown(); - redis_service_ptr->Stop(); + redis_service_ptr->Stop(); + DataSubstrate::Instance().Shutdown();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data_substrate(1 hunks)include/redis_service.h(1 hunks)src/redis_server.cpp(4 hunks)src/redis_service.cpp(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- data_substrate
🚧 Files skipped from review as they are similar to previous changes (1)
- include/redis_service.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/redis_service.cpp (2)
src/vector/vector_handler.cpp (4)
Instance(125-132)Instance(125-125)InitHandlerInstance(92-118)InitHandlerInstance(92-96)src/redis_server.cpp (1)
config_reader(441-441)
🔇 Additional comments (3)
src/redis_service.cpp (2)
6053-6054: LGTM!The transition from global DataSubstrate access to instance-based access via
DataSubstrate::Instance()is correct and aligns with the refactoring objectives.
186-258: The engine registration timing is correct as implemented.EnableEngine()(redis_server.cpp:461) enables the engine capability in DataSubstrate, andRegisterEngine()(redis_service.cpp:249) is subsequently called withinInit()to provide the actual implementation (tables, metrics, factory). This two-step pattern is intentional and logical—no conflict exists.src/redis_server.cpp (1)
452-500: LGTM! Multi-step startup sequence is well-structured.The new phased initialization sequence clearly separates concerns:
- Initialize DataSubstrate instance
- Enable and register the EloqKv engine
- Start DataSubstrate (brings up substrate engines)
- Start Redis service (binds runtime dependencies)
This provides better error handling and makes the dependencies explicit. Each step properly cleans up on failure by calling
Stop()andShutdown()in the correct order.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/redis_service.cpp (1)
446-570:Start()sequencing is good, buteloq_data_pathusage is still unresolved and will likely break the buildThe new
RedisServiceImpl::Start(brpc::Server &)generally follows a solid pattern:
- Attaches to
TxServiceand (optionally)DataStoreHandleronly afterDataSubstrate::Start().- Sizes slow‑log structures off
core_num_, and uses DS network config to deriveredis_ip_port.- Starts the metrics collector thread only when metrics are enabled.
- Handles bootstrap mode by calling
Stop(),ds.Shutdown(), and exiting.However, in the vector‑index initialization block (roughly Line 560), you still pass
eloq_data_pathintoEloqVec::VectorHandler::InitHandlerInstancewithout any visible declaration in this translation unit:if (!EloqVec::VectorHandler::InitHandlerInstance( tx_service_, vector_index_worker_pool_.get(), eloq_data_path, &vector_cloud_config))Unless
eloq_data_pathis declared in one of the included headers (as anexternor similar), this will be a compile‑time error. Even if it is declared elsewhere, you need to ensure that it is initialized beforeStart()is called and actually represents the correct vector‑index data path.You probably want either:
- To obtain the data path from
DataSubstrate(e.g., a core or storage config field), or- To store the required path as a member of
RedisServiceImplduringInit()(derived from the config file) and use that member here instead of a free variable.For example, if you decide to store it as a member:
- if (!EloqVec::VectorHandler::InitHandlerInstance( - tx_service_, - vector_index_worker_pool_.get(), - eloq_data_path, - &vector_cloud_config)) + if (!EloqVec::VectorHandler::InitHandlerInstance( + tx_service_, + vector_index_worker_pool_.get(), + eloq_data_path_, // member set in Init() from config + &vector_cloud_config))and add an appropriate
std::string eloq_data_path_;member plus initialization.#!/bin/bash # Verify that `eloq_data_path` is properly declared and defined. echo "=== Search for any declaration/definition of eloq_data_path ===" rg -n "eloq_data_path" --type cpp -C3 || true echo echo "If you only see uses inside redis_service.cpp and no declaration such as" echo "\"extern std::string eloq_data_path;\" in a header or a definition in another" echo "translation unit, you'll need to introduce a proper member or config-based" echo "source for this path as suggested in the review comment."src/redis_server.cpp (1)
534-535: Normal shutdown should stop Redis before tearing down DataSubstrate (reopens prior concern)Here the shutdown order is reversed compared to your error paths:
DataSubstrate::Instance().Shutdown(); redis_service_ptr->Stop();Since
RedisServiceImpl::Stop()is what stops and joins the metrics collector thread, which in turn uses metrics state initialized/owned byDataSubstrate, callingShutdown()first risks the metrics thread running against torn‑down DataSubstrate state during shutdown.To keep shutdown symmetric and avoid that race, stop Redis (and its background threads) before shutting down DataSubstrate:
- DataSubstrate::Instance().Shutdown(); - redis_service_ptr->Stop(); + redis_service_ptr->Stop(); + DataSubstrate::Instance().Shutdown();This matches the error paths above and the bootstrap path in
RedisServiceImpl::Start().
🧹 Nitpick comments (1)
src/redis_service.cpp (1)
186-257: Engine registration and metrics wiring via DataSubstrate look good; consider guardingdatabasesfor reserveThe move to build
prebuilt_tablesandengine_metricsand register the EloqKv engine throughDataSubstrate::RegisterEngineis clean and keeps Redis‑specific setup localized inInit().One minor robustness point:
prebuilt_tables.reserve(databases);uses anintthat comes from config. Ifdatabaseswere misconfigured to a negative value, it would be converted to a hugesize_tand causereserve()to throw instead of simply resulting in zero tables as before. You could defensively clamp or guard it, e.g.:- std::vector<std::pair<txservice::TableName, std::string>> prebuilt_tables; - prebuilt_tables.reserve(databases); + std::vector<std::pair<txservice::TableName, std::string>> prebuilt_tables; + if (databases > 0) + { + prebuilt_tables.reserve(databases); + }Not critical, but would make behavior under bad config inputs more predictable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data_substrate(1 hunks)include/redis_service.h(1 hunks)src/redis_server.cpp(4 hunks)src/redis_service.cpp(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- data_substrate
🧰 Additional context used
🧬 Code graph analysis (1)
include/redis_service.h (1)
src/redis_service.cpp (2)
Start(446-570)Start(446-446)
🔇 Additional comments (4)
include/redis_service.h (1)
168-170: Two‑phaseStartAPI for RedisServiceImpl looks consistent with usageThe added
Start(brpc::Server &)and its docstring match the new startup sequence inredis_server.cpp; the lifecycle surface here looks good.src/redis_server.cpp (2)
452-500: Startup refactor (DataSubstrate Init/Start + Redis Init/Start) reads cleanSequencing
Init→EnableEngine(EloqKv)→DataSubstrate::Start()→RedisServiceImpl::Start(server)with consistent failure cleanup looks correct and matches the intended two‑phase initialization.
507-512: Error path onserver.Startpreserves safe teardown orderOn listen failure, calling
redis_service_ptr->Stop()beforeDataSubstrate::Instance().Shutdown()keeps the Redis metrics thread and other Redis runtime work from touching DataSubstrate after it's torn down. This ordering is appropriate.src/redis_service.cpp (1)
6053-6055: Using DataSubstrate core config for MaxConnectionCount is appropriate
MaxConnectionCount()now delegating toDataSubstrate::Instance().GetCoreConfig().maxclientskeeps the limit centralized in the substrate’s configuration and is consistent with the broader refactor. Given the startup sequence inredis_server.cpp, this should always be called after DataSubstrate is initialized and started.
liunyl
force-pushed
the
eloqdb
branch
2 times, most recently
from
abb339f to
92ade76
Compare
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.