Pass term to eloqstore during start data store.#318
Conversation
|
Warning Rate limit exceeded
⌛ 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 (1)
WalkthroughAdds an int64_t term parameter to leader-start callbacks and threads it through DataStoreService, DataStoreFactory, DataStore::StartDB implementations; extends AppendPreBuiltTable to accept a kv_table_name and updates prebuilt-table wiring and RocksDB handler engine propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant CCNode as CC Node
participant Handler as DataStoreHandler
participant Client as DataStoreServiceClient
participant Service as DataStoreService
participant Factory as DataStoreFactory
participant DS as DataStoreImpl
rect rgba(235,245,235,0.5)
Note over CCNode,Handler: Leader start event with term
end
CCNode->>Handler: OnLeaderStart(ng_id, term, next_leader_node)
Handler->>Client: OnLeaderStart(ng_id, term, next_leader_node)
Client->>Service: OpenDataStore(shard_id, bucket_ids, term)
Service->>Service: ConnectAndStartDataStore(shard_id, mode, create_if_missing, term)
Service->>Factory: CreateDataStore(create_if_missing, shard_id, svc, start_db=true, term)
Factory->>DS: instantiate DataStore
Factory->>DS: StartDB(term)
DS-->>Factory: Start result
Factory-->>Service: DataStore ptr
Service-->>Client: open result
Client-->>Handler: result
Handler-->>CCNode: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
♻️ Duplicate comments (1)
store_handler/rocksdb_handler.cpp (1)
2125-2149: Term parameter received but not used in StartDB call.The
termparameter is accepted in the signature but not passed toStartDB(true, next_leader_node)at line 2135. This breaks the term propagation chain that the PR establishes for other data store implementations.This is the same issue flagged in
rocksdb_handler.hline 543. Please see that comment for the verification script.
🧹 Nitpick comments (3)
store_handler/data_store_service_client.cpp (1)
3653-3666: Term propagation intoOpenDataStorelooks correct; consider surfacing failures.Forwarding
termhere alignsDataStoreServiceClient::OnLeaderStartwith the new term-aware startup flow and withOnLeaderStop(uint32_t ng_id, int64_t term). If you ever want leader election to react whenOpenDataStore(ng_id, term)fails (e.g., by retrying or stepping down), this would be the place to start checking and returning an error instead of always returningtrue, but that behavior was already present before this change.tx_service/include/store/data_store_handler.h (1)
410-415: Interface change makes leader hooks term-aware; consider documenting semantics.Extending
OnLeaderStartto(uint32_t ng_id, int64_t term, uint32_t *next_leader_node)brings it in line withOnLeaderStopand the RAFT term model. The change is sound; as a follow-up, a brief comment here clarifying thattermis the RAFT leader term (and how implementations should use it, if at all) would help implementers of customDataStoreHandlers.store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h (1)
48-55:CreateDataStoreterm wiring is correct; small comment/doc tweak suggested.
- Adding
start_dbandint64_t termand callingds->StartDB(term)whenstart_dbis true correctly makes RocksDB Cloud startup term-aware while preserving existing behavior via defaults.- The comment above still says “the user can include shard path in the URL themselves”, but this factory always appends
/ds_<shard_id>tooss_url_, so including the shard path in the URL would lead to a duplicated suffix. It would be clearer to state that callers should not include the shard component inoss_urland let the factory add/ds_<shard_id>itself. Based on learnings, this matches the intended usage.Also applies to: 93-99
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloq_store_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.cppstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store.cppstore_handler/eloq_data_store_service/rocksdb_data_store.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/rocksdb_handler.cppstore_handler/rocksdb_handler.htx_service/include/store/data_store_handler.htx_service/src/fault/cc_node.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
store_handler/data_store_service_client.hstore_handler/data_store_service_client.cpptx_service/src/fault/cc_node.cppstore_handler/eloq_data_store_service/data_store_service.cpp
📚 Learning: 2025-12-16T09:47:09.981Z
Learnt from: githubzilla
Repo: eloqdata/tx_service PR: 288
File: store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h:54-76
Timestamp: 2025-12-16T09:47:09.981Z
Learning: In RocksDBCloudDataStoreFactory::CreateDataStore (store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h), when oss_url is configured, the factory always appends /ds_<shard_id> to the oss_url. Users should not include the shard path in the oss_url themselves.
Applied to files:
store_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cpp
🧬 Code graph analysis (7)
store_handler/eloq_data_store_service/rocksdb_data_store.h (3)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
store_handler/eloq_data_store_service/data_store_service.h (1)
store_handler/eloq_data_store_service/data_store_service.cpp (2)
OpenDataStore(2204-2238)OpenDataStore(2204-2204)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
EloqStoreDataStore(162-269)
store_handler/rocksdb_handler.h (3)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3653-3671)OnLeaderStart(3653-3655)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2125-2149)OnLeaderStart(2125-2125)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-470)OnLeaderStart(257-260)
store_handler/rocksdb_handler.cpp (2)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3653-3671)OnLeaderStart(3653-3655)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-470)OnLeaderStart(257-260)
store_handler/eloq_data_store_service/eloq_store_data_store.h (2)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
🔇 Additional comments (18)
store_handler/eloq_data_store_service/data_store.h (1)
59-63: LGTM! Clean interface extension.The addition of the
termparameter to theStartDBinterface is well-documented and propagated consistently across all implementing classes in this PR.store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-271: Term parameter unused, consistent with cloud variant.Similar to
RocksDBCloudDataStore::StartDB, thetermparameter is not used in this implementation. This appears to be a consistent pattern where the term infrastructure is added but only utilized by specific implementations (e.g., EloqStoreDataStore).store_handler/data_store_service_client.h (1)
445-445: LGTM! OnLeaderStart signature extended with term parameter.The term parameter is appropriately positioned in the signature between the node group ID and the output parameter, maintaining logical parameter ordering.
store_handler/eloq_data_store_service/rocksdb_data_store.h (1)
46-51: LGTM! Header declaration consistent with implementation.The signature and documentation are properly updated to match the implementation changes.
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
174-183: LGTM! Term parameter correctly propagated to EloqStore.This is the implementation where the
termparameter is actually utilized - it's passed through toeloq_store_service_->Start(term)on line 176. This confirms the term infrastructure is functional and aligns with the PR's goal of enabling term-aware startup for EloqStore.store_handler/eloq_data_store_service/rocksdb_cloud_data_store.h (1)
60-65: LGTM! Header declaration properly updated.The signature and documentation changes are consistent with the implementation in
rocksdb_cloud_data_store.cpp.store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
263-263: Thetermparameter is unused in the function implementation.The function signature includes
int64_t term, but this parameter is never referenced anywhere in the body ofStartDB()(lines 263-486). Instead, the function discovers the term value independently by callingFindMaxTermFromCloudManifestFiles()and usesmax_term + 1for cookie generation.This suggests the parameter may be intended for future use or as part of an ongoing refactoring, but currently it serves no purpose. Either remove the parameter if it's not needed, or add a comment explaining why it's present and when it should be used.
store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
44-58: LGTM! Term parameter correctly propagated to StartDB.The factory now accepts the term parameter and properly passes it to
StartDB(term)at line 52, enabling term-aware startup for EloqStore. The default value of 0 is appropriate for bootstrap scenarios.store_handler/eloq_data_store_service/data_store_service.h (1)
652-652: LGTM! Term propagation added to DataStoreService API.The
OpenDataStoreandConnectAndStartDataStoresignatures now accept term and properly propagate it through the call chain (visible in data_store_service.cpp snippet at line 2217).store_handler/eloq_data_store_service/rocksdb_data_store_factory.h (1)
45-72: LGTM! Factory correctly propagates term to RocksDBDataStore.The implementation properly threads the term parameter through to
StartDB(term)at line 63, aligning with the term-aware startup pattern established across data store factories.store_handler/eloq_data_store_service/data_store_factory.h (1)
44-49: LGTM! Base interface updated to support term-aware data store creation.The pure virtual method signature now includes the term parameter with a default value, establishing the contract that all factory implementations must follow for term propagation.
store_handler/rocksdb_handler.h (1)
543-543: Confirm whether the term parameter in OnLeaderStart is intentionally unused.The
int64_t termparameter inRocksDBHandler::OnLeaderStartis not used in the implementation. The method callsStartDB(true, next_leader_node)without passing the term parameter, andRocksDBHandler::StartDB(rocksdb_handler.h lines 556, 700, 756) doesn't accept a term parameter at all. This differs from theDataStorehierarchy whereStartDB(int64_t term)is required.Clarify whether:
- This is intentional because RocksDBHandler operates at a different abstraction level and doesn't require term tracking at database initialization, or
- The implementation should propagate term to StartDB to maintain consistency with the DataStore hierarchy design pattern.
store_handler/eloq_data_store_service/data_store_service.cpp (5)
292-295: LGTM: Appropriate default term for bootstrap.Using
term=0for bootstrap or single-node deployment is a reasonable default when term information is unavailable. The comment clearly documents this intent.
404-405: LGTM: Term correctly propagated to CreateDataStore.The term parameter is properly passed through to
CreateDataStore, consistent with the refactoring objective.
421-421: LGTM: Term properly passed to StartDB.The term is correctly propagated to
StartDB, fulfilling the PR's objective to pass term during data store startup.
2204-2238: LGTM: Term parameter correctly threaded through OpenDataStore.The
OpenDataStoresignature has been appropriately extended to accept atermparameter, which is then correctly propagated toConnectAndStartDataStore. The implementation maintains the existing timing and logging logic while properly passing the term value through the call chain.
361-364: No compilation error at line 1777.The
ConnectAndStartDataStorefunction signature (line 672-675) declares default parameters for bothcreate_db_if_missing = falseandterm = 0. The call at line 1777 passesfalsefor the third argument and relies on the defaultterm = 0for the fourth argument, which is valid C++ syntax.Likely an incorrect or invalid review comment.
tx_service/src/fault/cc_node.cpp (1)
353-361: Correctly threads RAFT term into the data store handler.Passing
termtostore_hd_->OnLeaderStart(ng_id_, term, next_leader_node)keeps the data-store side in sync with the RAFT leader term you later record inSharder, and matches the updated handler interface. No further changes needed here.
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
from
22fd0f5 to
6d7807f
Compare
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)
store_handler/eloq_data_store_service/data_store_service.cpp (1)
1777-1777: Add term parameter to OpenDSShardRequest and propagate it to ConnectAndStartDataStore.The call to
ConnectAndStartDataStoreon line 1777 doesn't pass atermargument, causing it to default to 0. The underlying issue is thatOpenDSShardRequestprotobuf message lacks atermfield, making it impossible for theOpenDSShardRPC handler to propagate the term even if it were provided.Given that:
OpenDataStoremethod (line 2218) correctly passes term toConnectAndStartDataStore- The PR adds term-aware startup throughout the system
- Shard opening operations are part of leader-driven flows that may require term information
The fix requires:
- Add
int64_t termfield toOpenDSShardRequestprotobuf- Extract term from the request and pass it to
ConnectAndStartDataStoreon line 1777
🧹 Nitpick comments (1)
store_handler/eloq_data_store_service/data_store_factory.h (1)
44-49: Add documentation for thetermparameter.All implementations have been properly updated with the new parameter. However, add a doc comment explaining the purpose and semantics of the
termparameter (e.g., what consensus term represents, when it's used, and why the default is 0).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloq_store_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.cppstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store.cppstore_handler/eloq_data_store_service/rocksdb_data_store.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/rocksdb_handler.cppstore_handler/rocksdb_handler.htx_service/include/store/data_store_handler.htx_service/src/fault/cc_node.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- store_handler/rocksdb_handler.cpp
- store_handler/data_store_service_client.cpp
- store_handler/eloq_data_store_service/rocksdb_cloud_data_store.h
- store_handler/eloq_data_store_service/rocksdb_data_store.h
- tx_service/src/fault/cc_node.cpp
- store_handler/eloq_data_store_service/rocksdb_data_store_factory.h
- store_handler/eloq_data_store_service/data_store.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-16T09:47:09.981Z
Learnt from: githubzilla
Repo: eloqdata/tx_service PR: 288
File: store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h:54-76
Timestamp: 2025-12-16T09:47:09.981Z
Learning: In RocksDBCloudDataStoreFactory::CreateDataStore (store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h), when oss_url is configured, the factory always appends /ds_<shard_id> to the oss_url. Users should not include the shard path in the oss_url themselves.
Applied to files:
store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cpp
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
store_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store_service.cpp
🧬 Code graph analysis (7)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
tx_service/include/store/data_store_handler.h (3)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3657-3675)OnLeaderStart(3657-3659)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2125-2149)OnLeaderStart(2125-2125)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-470)OnLeaderStart(257-260)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)
store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
EloqStoreDataStore(162-269)
store_handler/eloq_data_store_service/data_store_service.h (1)
store_handler/eloq_data_store_service/data_store_service.cpp (2)
OpenDataStore(2204-2238)OpenDataStore(2204-2204)
store_handler/data_store_service_client.h (4)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3657-3675)OnLeaderStart(3657-3659)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2125-2149)OnLeaderStart(2125-2125)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-470)OnLeaderStart(257-260)tx_service/src/sharder.cpp (2)
OnLeaderStart(836-852)OnLeaderStart(836-840)
store_handler/rocksdb_handler.h (3)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3657-3675)OnLeaderStart(3657-3659)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2125-2149)OnLeaderStart(2125-2125)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-470)OnLeaderStart(257-260)
🔇 Additional comments (13)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-85: Signature update aligns with interface contract.The
termparameter is added for interface consistency with the baseDataStoreclass. RocksDB-based implementations don't require term information for startup (unlikeEloqStoreDataStorewhich passes it toeloq_store_service_->Start(term)), so the unused parameter is acceptable here.store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
174-183: Term correctly propagated to EloqStore service.The implementation properly forwards the
termparameter toeloq_store_service_->Start(term), enabling term-aware startup for EloqStore. Error handling with logging and appropriate return value is in place.tx_service/include/store/data_store_handler.h (1)
410-415: Interface correctly extended with term parameter.The
OnLeaderStartsignature is updated to include theint64_t termparameter, enabling term-aware leader startup across data store implementations. The default implementation appropriately returnstruefor handlers that don't require special leader-start logic.store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
44-57: Factory correctly propagates term to StartDB.The
CreateDataStoremethod properly accepts thetermparameter with a default value of0for backward compatibility, and correctly forwards it tods->StartDB(term)whenstart_dbis true.store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
263-263: Signature update for interface consistency.The
termparameter is added for interface consistency with theDataStorebase class. The RocksDB Cloud implementation doesn't require this term for its startup logic, similar to the standard RocksDB implementation. Note that the existing "term" logic in cloud manifest handling (lines 431-466) operates independently and refers to cloud manifest versioning, not the leadership term being passed here.store_handler/eloq_data_store_service/data_store_service.h (2)
652-652: OpenDataStore correctly updated with term parameter.The method signature is properly updated to accept
int64_t termwith a default value of0for backward compatibility. Per the implementation indata_store_service.cpp, the term is correctly propagated toConnectAndStartDataStore.
672-675: ConnectAndStartDataStore signature correctly extended.The method signature properly includes the
termparameter, which will be used to create the data store with term-awareness via the factory'sCreateDataStore(create_db_if_missing, data_shard_id, this, true, term)call.store_handler/rocksdb_handler.h (1)
543-543: OnLeaderStart signature correctly updated for interface compliance.The method signature is updated to match the
DataStoreHandlerinterface. The implementation (perrocksdb_handler.cpp) doesn't currently use thetermparameter, which is consistent with RocksDB-based implementations not requiring term information for their startup logic.store_handler/data_store_service_client.h (1)
445-445: OnLeaderStart correctly updated to propagate term.The signature is properly updated to include the
int64_t termparameter. Per the implementation indata_store_service_client.cpp, the term is correctly forwarded viadata_store_service_->OpenDataStore(ng_id, term), completing the term propagation chain from leader election through to the data store startup.store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h (1)
52-53: LGTM! Term parameter properly propagated.The addition of the
termparameter with a default value of 0 maintains backward compatibility while enabling term-aware startup. The term is correctly forwarded toStartDB(term)whenstart_dbis true.Also applies to: 95-95
store_handler/eloq_data_store_service/data_store_service.cpp (3)
292-295: Term 0 for bootstrap is documented and appears correct.The explicit comment clarifies that term 0 is used for bootstrap or single-node deployment scenarios, which aligns with typical consensus system semantics where term 0 represents an initial or non-replicated state.
361-364: LGTM! Term correctly propagated through ConnectAndStartDataStore.The term parameter is properly threaded to both the
CreateDataStorefactory method (line 405) and theStartDBcall (line 421), ensuring term-aware startup whether creating a new store or restarting an existing one.Also applies to: 405-405, 421-421
2204-2204: LGTM! OpenDataStore correctly accepts and propagates term.The term parameter is properly added to the signature and correctly forwarded to
ConnectAndStartDataStore, maintaining the term propagation chain.Also applies to: 2218-2219
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
from
6d7807f to
b37afd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-271: Marktermas intentionally unused inRocksDBDataStore::StartDB
int64_t termis currently unused; behavior matches the old implementation, but it may raise compiler warnings. Consider explicitly marking it unused.Possible tweak
-bool RocksDBDataStore::StartDB(int64_t term) +bool RocksDBDataStore::StartDB(int64_t /*term*/) {or add
static_cast<void>(term);at the top of the function.store_handler/rocksdb_handler.cpp (1)
2125-2149: Handle unusedtermparameter inRocksDBHandler::OnLeaderStartThe new
termargument is not used; behavior is unchanged, but this can trigger unused-parameter warnings. If RocksDB really doesn’t need the term here, consider marking it unused:bool RocksDBHandler::OnLeaderStart(uint32_t /*ng_id*/, int64_t /*term*/, uint32_t *next_leader_node)(or cast them to void at the top).
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
263-486: Consider markingtermunused inRocksDBCloudDataStore::StartDB
StartDB(int64_t term)doesn’t useterm; the startup logic still relies on manifest-derivedmax_term. That’s fine semantically, but you may want to marktermas intentionally unused (e.g.,int64_t /*term*/orstatic_cast<void>(term);) to keep builds warning-free.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloq_store_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.cppstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store.cppstore_handler/eloq_data_store_service/rocksdb_data_store.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/rocksdb_handler.cppstore_handler/rocksdb_handler.htx_service/include/store/data_store_handler.htx_service/src/fault/cc_node.cpp
🚧 Files skipped from review as they are similar to previous changes (8)
- store_handler/eloq_data_store_service/rocksdb_data_store.h
- store_handler/eloq_data_store_service/data_store_factory.h
- tx_service/include/store/data_store_handler.h
- store_handler/data_store_service_client.cpp
- store_handler/data_store_service_client.h
- store_handler/rocksdb_handler.h
- store_handler/eloq_data_store_service/data_store.h
- store_handler/eloq_data_store_service/rocksdb_cloud_data_store.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
tx_service/src/fault/cc_node.cppstore_handler/eloq_data_store_service/data_store_service.cpp
📚 Learning: 2025-12-16T09:47:09.981Z
Learnt from: githubzilla
Repo: eloqdata/tx_service PR: 288
File: store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h:54-76
Timestamp: 2025-12-16T09:47:09.981Z
Learning: In RocksDBCloudDataStoreFactory::CreateDataStore (store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h), when oss_url is configured, the factory always appends /ds_<shard_id> to the oss_url. Users should not include the shard path in the oss_url themselves.
Applied to files:
store_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cpp
🧬 Code graph analysis (5)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (3)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/rocksdb_handler.cpp (4)
StartDB(2427-2505)StartDB(2427-2428)StartDB(2973-3396)StartDB(2973-2973)
store_handler/eloq_data_store_service/eloq_store_data_store.h (3)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)store_handler/rocksdb_handler.cpp (4)
StartDB(2427-2505)StartDB(2427-2428)StartDB(2973-3396)StartDB(2973-2973)
store_handler/eloq_data_store_service/data_store_service.h (1)
store_handler/eloq_data_store_service/data_store_service.cpp (2)
OpenDataStore(2204-2238)OpenDataStore(2204-2204)
store_handler/rocksdb_handler.cpp (2)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3657-3675)OnLeaderStart(3657-3659)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-476)OnLeaderStart(257-260)
🔇 Additional comments (14)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
174-183: Term-aware StartDB implementation for EloqStore looks correct
StartDB(int64_t term)cleanly forwards the term toeloq_store_service_->Start(term)and preserves existing error handling and return semantics. No issues from this change.store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
40-58: Factory wiring ofterminto EloqStore startup is consistent
EloqStoreDataStoreFactory::CreateDataStorenow acceptstermand passes it toStartDB(term)whenstart_dbis true, preserving existing failure handling and keepingtermoptional via a default. This looks correct.tx_service/src/fault/cc_node.cpp (1)
257-311: Correctly propagating Raft term into data-store OnLeaderStart
CcNode::OnLeaderStartnow forwards thetermargument intostore_hd_->OnLeaderStart(ng_id_, term, next_leader_node), aligning the CC layer with the updated term-aware data store handler interface without changing existing retry/early-exit logic.store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h (1)
48-104: Term parameter is cleanly wired through the cloud data store factory
RocksDBCloudDataStoreFactory::CreateDataStorenow acceptstermand forwards it tods->StartDB(term)whenstart_dbis true, preserving the existing shard-specific path/URL handling and error logging. Change looks correct and backwards compatible.store_handler/eloq_data_store_service/data_store_service.h (1)
651-676: DataStoreService APIs now expose term consistentlyAdding an optional
int64_t termtoOpenDataStoreandConnectAndStartDataStorealigns the service interface with the term-aware startup path, while defaulting to 0 for existing non-Raft callers. The header matches the described implementation wiring indata_store_service.cpp.store_handler/eloq_data_store_service/rocksdb_data_store_factory.h (2)
49-50: LGTM! Backward-compatible term parameter addition.The default parameter
term = 0maintains backward compatibility while enabling term-aware data store initialization.
63-63: LGTM! Term correctly propagated to StartDB.The term parameter is properly passed through to the data store's StartDB method.
store_handler/eloq_data_store_service/data_store_service.cpp (7)
292-295: Bootstrap term propagation looks correct.The comment clarifies that term=0 is used for bootstrap or single-node deployments. This parameter is now correctly passed to
CreateDataStore.
404-406: LGTM! Term correctly passed to CreateDataStore.When creating a new data store, the term is properly propagated through the factory.
421-421: LGTM! Term correctly passed to StartDB for existing data store.When reinitializing an existing data store, the term is properly propagated to
StartDB.
2218-2219: LGTM! Term correctly propagated in OpenDataStore.The
ConnectAndStartDataStorecall properly includes all four parameters including the term value.
1777-1777: No action needed. The function signature at line 672-675 in the header file shows that thetermparameter has a default value of0:bool ConnectAndStartDataStore(uint32_t data_shard_id, DSShardStatus open_mode, bool create_db_if_missing = false, int64_t term = 0);The call at line 1777 with three arguments is valid and will compile successfully, as the fourth parameter uses its default value.
Likely an incorrect or invalid review comment.
361-364: No action required—signature change is backward-compatible.The header file declares
int64_t term = 0as a default value, making this a backward-compatible change. Existing call sites that don't passtermwill use the default value. All call sites in the codebase are valid.
2204-2204: OpenDataStore signature extended with term parameter.All callers have been updated to pass the term parameter. The declaration includes a default value (
int64_t term = 0) for backward compatibility.
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
2 times, most recently
from
3b3c387 to
75f023c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@store_handler/eloq_data_store_service/data_store_service.cpp`:
- Around line 2204-2206: The function signature for
DataStoreService::OpenDataStore contains an invalid full-width Chinese comma
between the parameter types; replace the Chinese comma after the
rvalue-reference parameter "std::unordered_set<uint16_t> &&bucket_ids， int64_t
term" with a standard ASCII comma so the signature reads
"std::unordered_set<uint16_t> &&bucket_ids, int64_t term" to fix the compilation
error.
In `@store_handler/rocksdb_handler.h`:
- Line 548: The OnLeaderStart declaration currently takes parameters ng_id and
term that are unused; update the declaration/definition of OnLeaderStart (and
its implementation) to mark unused parameters with [[maybe_unused]] (e.g.,
[[maybe_unused]] uint32_t ng_id, [[maybe_unused]] int64_t term) or, if you
prefer not to change the signature, add a clear comment in the OnLeaderStart
implementation explaining why ng_id and term are intentionally unused and that
StartDB only uses is_ng_leader and next_leader; reference the OnLeaderStart
method and the StartDB call sites to ensure consistency.
🧹 Nitpick comments (3)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-92: Silence unusedtermto avoid warning noise.♻️ Optional tweak
bool RocksDBDataStore::StartDB(int64_t term) { + (void)term; if (db_) { // db is already started, no op DLOG(INFO) << "DB already started"; return true; }store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
263-270: Silence unusedtermto avoid warning noise.♻️ Optional tweak
bool RocksDBCloudDataStore::StartDB(int64_t term) { + (void)term; if (db_) { // db is already started, no op DLOG(INFO) << "DBCloud already started"; return true; }tx_service/src/fault/cc_node.cpp (1)
305-306: Minor formatting: add space after comma.The function call is missing a space after the first comma, which slightly deviates from typical C++ formatting style.
Suggested fix
- if (!local_cc_shards_.store_hd_->OnLeaderStart(ng_id_,term, + if (!local_cc_shards_.store_hd_->OnLeaderStart(ng_id_, term, next_leader_node))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloq_store_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.cppstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store.cppstore_handler/eloq_data_store_service/rocksdb_data_store.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/rocksdb_handler.cppstore_handler/rocksdb_handler.htx_service/include/store/data_store_handler.htx_service/src/fault/cc_node.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- store_handler/eloq_data_store_service/data_store_service.h
- store_handler/rocksdb_handler.cpp
- store_handler/eloq_data_store_service/rocksdb_cloud_data_store.h
- store_handler/data_store_service_client.cpp
- store_handler/eloq_data_store_service/rocksdb_data_store.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-16T09:47:09.981Z
Learnt from: githubzilla
Repo: eloqdata/tx_service PR: 288
File: store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h:54-76
Timestamp: 2025-12-16T09:47:09.981Z
Learning: In RocksDBCloudDataStoreFactory::CreateDataStore (store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h), when oss_url is configured, the factory always appends /ds_<shard_id> to the oss_url. Users should not include the shard path in the oss_url themselves.
Applied to files:
store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cpp
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
tx_service/src/fault/cc_node.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store_service.cpp
🧬 Code graph analysis (6)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
tx_service/include/store/data_store_handler.h (3)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3700-3728)OnLeaderStart(3700-3702)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2149-2173)OnLeaderStart(2149-2149)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-476)OnLeaderStart(257-260)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)
store_handler/eloq_data_store_service/data_store.h (3)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
store_handler/data_store_service_client.h (4)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3700-3728)OnLeaderStart(3700-3702)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2149-2173)OnLeaderStart(2149-2149)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-476)OnLeaderStart(257-260)tx_service/src/sharder.cpp (2)
OnLeaderStart(840-856)OnLeaderStart(840-844)
store_handler/rocksdb_handler.h (3)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3700-3728)OnLeaderStart(3700-3702)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2149-2173)OnLeaderStart(2149-2149)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-476)OnLeaderStart(257-260)
🪛 Cppcheck (2.19.0)
store_handler/eloq_data_store_service/data_store_service.cpp
[error] 2205-2205: The code contains unhandled character(s) (character code=239). Neither unicode nor extended ascii is supported.
(unhandledChar)
🔇 Additional comments (10)
store_handler/eloq_data_store_service/data_store.h (1)
58-63: LGTM — StartDB now accepts term with clear doc.store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
174-182: Good pass-through of term to EloqStore Start.store_handler/data_store_service_client.h (1)
456-456: Signature update looks consistent.tx_service/include/store/data_store_handler.h (1)
415-420: LGTM - Interface extended with term parameter.The base class virtual method signature is correctly updated to include the
int64_t termparameter. The default implementation maintains backward compatibility by providing a no-op return value.store_handler/eloq_data_store_service/data_store_service.cpp (2)
292-295: LGTM - Term propagation in StartService.Good documentation explaining why term=0 is used for bootstrap/single-node deployment. The term is correctly passed through to
CreateDataStore.
361-427: LGTM - ConnectAndStartDataStore updated with term parameter.The function correctly accepts and propagates the term parameter to both
CreateDataStoreandStartDBcalls, enabling term-aware data store initialization.store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
41-59: LGTM - CreateDataStore correctly propagates term to StartDB.The factory method is properly updated to accept the
termparameter with a sensible default value of 0, and correctly forwards it toStartDB(term)when starting the database. This aligns with the PR objective of passing term to eloqstore during data store startup.store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h (1)
48-53: LGTM! Term parameter properly added and propagated.The
CreateDataStoresignature correctly adds theint64_t termparameter with a default value of0, and the term is properly propagated toStartDB(term)whenstart_dbis true.Also applies to: 93-95
store_handler/eloq_data_store_service/rocksdb_data_store_factory.h (1)
45-50: LGTM! Consistent with base class and sibling factory implementations.The signature update and term propagation to
StartDB(term)are correctly implemented, matching the pattern inRocksDBCloudDataStoreFactory.Also applies to: 61-63
store_handler/eloq_data_store_service/data_store_factory.h (1)
45-50: Base class interface change fully implemented across all derived factories.The abstract
CreateDataStoremethod now includesint64_t term = 0parameter. All three derived factory classes (RocksDBDataStoreFactory, RocksDBCloudDataStoreFactory, and EloqStoreDataStoreFactory) have been updated with matching signatures. Call sites in data_store_service.cpp are compatible—one passes explicit 0, the other passes a term variable. The default parameter value maintains backward compatibility.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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)
store_handler/eloq_data_store_service/data_store_service.cpp (1)
1775-1785: Missingtermparameter inConnectAndStartDataStorecall.This call site at line 1777 passes only 3 arguments, but the updated
ConnectAndStartDataStoresignature requires 4 parameters (includingterm). This will cause a compilation error unless there's a default parameter value defined in the header.If this is intentional (e.g., a default parameter), consider adding a comment explaining the term semantics for the migration path. If not, the term value should be passed here—potentially from the RPC request or as a default value appropriate for shard migration.
Suggested fix (if term should default to 0 for migration)
// Connect before setting cluster config to avoid visiting data store when // it is not ready yet. - bool res = ConnectAndStartDataStore(shard_id, mode, false); + bool res = ConnectAndStartDataStore(shard_id, mode, false, 0);
🧹 Nitpick comments (2)
tx_service/src/fault/cc_node.cpp (1)
303-311: LGTM!The term is correctly propagated from the Raft leader start callback to the store handler, enabling term-aware data store initialization.
Minor nit: consider adding a space after the comma for consistency:
OnLeaderStart(ng_id_, term, next_leader_node).🔧 Suggested formatting fix
- if (!local_cc_shards_.store_hd_->OnLeaderStart(ng_id_,term, + if (!local_cc_shards_.store_hd_->OnLeaderStart(ng_id_, term,store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-92: Consider silencing the unusedtermparameter.
termisn’t used yet in this implementation. If unused-parameter warnings are treated as errors, add a no-op or attribute.♻️ Suggested tweak
bool RocksDBDataStore::StartDB(int64_t term) { + (void)term; if (db_) { // db is already started, no op DLOG(INFO) << "DB already started"; return true;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloq_store_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.cppstore_handler/eloq_data_store_service/rocksdb_cloud_data_store.hstore_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store.cppstore_handler/eloq_data_store_service/rocksdb_data_store.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/rocksdb_handler.cppstore_handler/rocksdb_handler.htx_service/include/store/data_store_handler.htx_service/src/fault/cc_node.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- store_handler/eloq_data_store_service/eloq_store_data_store.h
- store_handler/eloq_data_store_service/data_store_factory.h
- store_handler/data_store_service_client.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-16T09:47:09.981Z
Learnt from: githubzilla
Repo: eloqdata/tx_service PR: 288
File: store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h:54-76
Timestamp: 2025-12-16T09:47:09.981Z
Learning: In RocksDBCloudDataStoreFactory::CreateDataStore (store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h), when oss_url is configured, the factory always appends /ds_<shard_id> to the oss_url. Users should not include the shard path in the oss_url themselves.
Applied to files:
store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.hstore_handler/eloq_data_store_service/rocksdb_data_store_factory.hstore_handler/eloq_data_store_service/data_store_service.cpp
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
store_handler/data_store_service_client.cpptx_service/src/fault/cc_node.cppstore_handler/eloq_data_store_service/data_store_service.cpp
🧬 Code graph analysis (7)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.h (3)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
tx_service/include/store/data_store_handler.h (3)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3700-3728)OnLeaderStart(3700-3702)store_handler/rocksdb_handler.cpp (2)
OnLeaderStart(2149-2173)OnLeaderStart(2149-2149)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-476)OnLeaderStart(257-260)
store_handler/eloq_data_store_service/data_store.h (3)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
store_handler/eloq_data_store_service/rocksdb_data_store.h (4)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)store_handler/rocksdb_handler.cpp (4)
StartDB(2451-2529)StartDB(2451-2452)StartDB(2997-3485)StartDB(2997-2997)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
StartDB(85-271)StartDB(85-85)
store_handler/rocksdb_handler.cpp (2)
store_handler/data_store_service_client.cpp (2)
OnLeaderStart(3700-3728)OnLeaderStart(3700-3702)tx_service/src/fault/cc_node.cpp (2)
OnLeaderStart(257-476)OnLeaderStart(257-260)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (2)
store_handler/eloq_data_store_service/eloq_store_data_store.h (1)
StartDB(174-183)store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
StartDB(263-263)
🔇 Additional comments (17)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store.h (1)
60-65: LGTM!The method signature and documentation are correctly updated to include the
termparameter, consistent with the base class interface and the broader PR changes.store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
263-270: Thetermparameter is accepted but not used.The
termparameter is added to the signature but is not utilized in the implementation. In contrast,EloqStoreDataStore::StartDBpasses the term toeloq_store_service_->Start(term).If the term is intended to be used (e.g., for cloud manifest cookies or logging), consider incorporating it. If this is intentional for interface consistency, consider adding a comment or using
[[maybe_unused]]to suppress compiler warnings.store_handler/eloq_data_store_service/data_store.h (1)
58-63: LGTM!The base class interface is correctly updated to include the
termparameter with proper documentation. This establishes the contract for all derived data store implementations.tx_service/include/store/data_store_handler.h (1)
415-420: LGTM!The
OnLeaderStartsignature is correctly updated to include thetermparameter. The default implementation returningtruemaintains backward compatibility for handlers that don't require term-aware logic.store_handler/rocksdb_handler.cpp (1)
2149-2173: LGTM: async leader-start path waits for DB start completion.The condvar wait ensures
StartDBfinishes before returning the result.store_handler/eloq_data_store_service/eloq_store_data_store_factory.h (1)
41-54: LGTM: term is now threaded into StartDB.store_handler/rocksdb_handler.h (1)
548-548: LGTM: signature updated for term-aware leader start.store_handler/eloq_data_store_service/rocksdb_data_store.h (1)
46-51: LGTM: StartDB docs/signature updated for term.store_handler/data_store_service_client.cpp (1)
3700-3723: Term is correctly threaded into OpenDataStore.No further concerns here.
store_handler/eloq_data_store_service/data_store_service.h (1)
651-677: API signature update looks consistent.Default
term = 0preserves existing callers while enabling term-aware startup.store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h (1)
48-101: Term forwarding to StartDB looks good.The new parameter is plumbed through cleanly without altering startup semantics.
store_handler/eloq_data_store_service/rocksdb_data_store_factory.h (1)
45-69: StartDB term propagation is handled correctly here.No additional issues found.
store_handler/eloq_data_store_service/data_store_service.cpp (5)
292-295: LGTM!The comment clearly explains the rationale for using term 0 during bootstrap or single node deployment. The term parameter is correctly threaded through to
CreateDataStore.
404-427: LGTM!The term parameter is correctly propagated through both code paths:
- When
data_store_is null:CreateDataStore(..., term)is called- When
data_store_already exists:StartDB(term)is called after initializationThis ensures consistent term handling regardless of the data store's initial state.
2204-2206: LGTM!The
OpenDataStoresignature correctly includes the newint64_t termparameter. The previously flagged Chinese comma issue has been addressed.
2226-2227: LGTM!The
termparameter is correctly propagated fromOpenDataStoretoConnectAndStartDataStore, completing the term threading chain.
361-364: The function signature change is correct and properly backward compatible. The header file declaresConnectAndStartDataStorewithint64_t term = 0as a default parameter, so the call at line 1777 that only passes three arguments is valid and will not cause a compilation error. The signature allows both call patterns: explicit term argument (line 2226) and implicit default of 0 (line 1777).Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
2 times, most recently
from
1d6ae7b to
4ab3003
Compare
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)
store_handler/eloq_data_store_service/rocksdb_cloud_data_store_factory.h (1)
52-81: Clarify oss_url shard-path behavior in comment.The code appends
/ds_<shard_id>even whenoss_url_is configured (lines 73-81), but the comment says users can include the shard path themselves. That’s misleading and risks double shard paths. Update the comment (or the behavior) to match reality.📝 Suggested comment fix
- // When oss_url is configured, the user can include shard path in the - // URL themselves + // When oss_url is configured, the factory appends /ds_<shard_id>, + // so users should NOT include the shard path in oss_url.Based on learnings, the factory appends /ds_<shard_id> even when oss_url is configured.
♻️ Duplicate comments (1)
store_handler/rocksdb_handler.cpp (1)
2149-2166: Mark unusedterm(andng_id) or document intent.
term(andng_id) are currently unused in the implementation, which can trigger-Wunused-parameterand obscures intent. Consider adding[[maybe_unused]]or a brief comment clarifying why the term isn’t needed here.🛠️ Possible fix
-bool RocksDBHandler::OnLeaderStart(uint32_t ng_id, - int64_t term, - uint32_t *next_leader_node) +bool RocksDBHandler::OnLeaderStart([[maybe_unused]] uint32_t ng_id, + [[maybe_unused]] int64_t term, + uint32_t *next_leader_node)
🧹 Nitpick comments (1)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-86: Mark unusedtermto avoid unused-parameter warnings.
Line 85 introducestermbut it’s unused in this implementation. If your build treats unused parameters as errors, this can fail—consider explicitly marking it unused.♻️ Suggested tweak
bool RocksDBDataStore::StartDB(int64_t term) { + (void)term;
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
2 times, most recently
from
edcba20 to
bccb10b
Compare
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
from
bccb10b to
c519265
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@store_handler/rocksdb_handler.cpp`:
- Around line 680-682: The write path is using a different key than the read
path; change the construction of table_key in the write logic to include the
engine prefix the same way the read uses it — build table_key using
txservice::KvTablePrefixOf(table_engine) + table_name_str + "_catalog" (matching
catalog_cf_name) so both reads and writes use the same key; ensure you reference
the same variables table_engine and table_name_str when updating the write-site.
♻️ Duplicate comments (1)
store_handler/rocksdb_handler.h (1)
550-552: Unusedng_idandtermparameters.Neither
ng_idnortermare used in theRocksDBHandler::OnLeaderStartimplementation—the lambda only capturesnext_leader_node. Since this is part of the interface contract for other implementations (e.g., EloqStore), consider adding[[maybe_unused]]attributes or a brief comment explaining why these parameters are intentionally unused in the RocksDB context.
…rvices This commit updates the StartDB method in various data store classes to accept an additional parameter, `term`, allowing for more flexible database initialization. Changes include updates to method signatures and implementations in `data_store.h`, `rocksdb_data_store.h`, `rocksdb_cloud_data_store.h`, and their respective source files. The data store factory methods are also modified to pass the term value during creation and startup of data stores. Use term 0 to start data store in DataStoreService::StartService()
lzxddz
force-pushed
the
eloqstore-multi-write-term
branch
from
0d1c9d0 to
b433633
Compare
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)
store_handler/rocksdb_handler.cpp (1)
2158-2183: Thetermparameter is received but not used in the function.The
termparameter is passed toOnLeaderStartbut is never utilized. Line 2170 callsStartDB(true, next_leader_node)without passingterm. According to the PR objective ("Pass term to eloqstore during start data store"), thetermshould be propagated through to the data store initialization logic, but the current implementation ignores it.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
store_handler/eloq_data_store_service/rocksdb_data_store.cpp (1)
85-92: Silence unusedtermto avoid warning-as-error builds.The new parameter isn’t used in this implementation; some toolchains treat this as an error.
🔧 Proposed fix
bool RocksDBDataStore::StartDB(int64_t term) { + (void)term; if (db_) { // db is already started, no op DLOG(INFO) << "DB already started";
🤖 Fix all issues with AI agents
In `@store_handler/rocksdb_handler.cpp`:
- Around line 679-683: In RestoreTxCache, the catalog key is built without the
engine prefix (it uses table_name.String() + "_catalog"), so update the key
construction to include the table engine prefix like the other places: use
KvTablePrefixOf(table_engine) + table_name_str + "_catalog" (or the same
prefixed variable used in the surrounding code) when forming catalog_cf_name so
prefixed catalog entries are correctly found; ensure you reference the existing
table_engine and table_name_str variables and mirror the construction used in
the earlier blocks.
2 participants
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.