Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Supports user-defined playback points #4

Merged

Conversation

panlei-coder
Copy link
Collaborator

@panlei-coder panlei-coder commented Apr 18, 2024

让braft支持自定义设置节点重启和正常安装快照时的日志回放点。(pikiwidb层的代码修改见pr:https://github.com/OpenAtomFoundation/pikiwidb/pull/319)
(1)pikiwidb可以在重载的on_snapshot_load接口中根据当前是否是节点重启来设置日志的回放点,因此需要提交设置回放点的接口set_self_playback_point,并在后续的FSMCaller::do_snapshot_load接口中判断之前自定义设置的回放点是否比快照中保存的snapshot_index新,如果没有snapshot_index新,则以snapshot_index作为回放点。

void FSMCaller::do_snapshot_load(LoadSnapshotClosure* done) {
    SnapshotMeta meta;
    int ret = reader->load_meta(&meta);
    // ... 
    ret = _fsm->on_snapshot_load(reader);
    // ...

    if (meta.old_peers_size() == 0) {
        // Joint stage is not supposed to be noticeable by end users.
        Configuration conf;
        for (int i = 0; i < meta.peers_size(); ++i) {
            conf.add_peer(meta.peers(i));
        }
        _fsm->on_configuration_committed(conf, meta.last_included_index());
    }

    **// If the log playback point is not customized or is configured but smaller than that
    // in the snapshot(Consider booting when the node is normally shut down), 
    // the log playback point in the snapshot prevails.
    if (last_applied_index() < meta.last_included_index()) {
        _last_applied_index.store(meta.last_included_index(),
                              butil::memory_order_release);
        _last_applied_term = meta.last_included_term();
    }**
    
    done->Run();
}

(2)在生成快照时考虑直接将快照中的last_applied_index和last_applied_term设置为min(all column family flushed index)(这里在那时假设时以一个RocksDB一个Raft),需要提供根据log_index来获取对应term的接口。

@panlei-coder panlei-coder requested a review from KKorpse May 18, 2024 11:30
@AlexStocks
Copy link

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes across multiple files and functions, impacting core functionality related to log playback points in a distributed system. Understanding the implications of these changes requires a good grasp of the system's snapshot and log management mechanisms.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The method set_self_playback_point in NodeImpl does not handle the case where self_playback_point is less than or equal to _fsm_caller->last_applied_index() or greater than _log_manager->last_log_index(). This could potentially lead to setting an invalid playback point without any feedback or error.

🔒 Security concerns

No

Code feedback:
relevant filesrc/braft/node.cpp
suggestion      

Consider adding error handling or feedback mechanism in NodeImpl::set_self_playback_point when the playback point is not set due to invalid conditions. This could be crucial for debugging and ensuring system stability. [important]

relevant lineif (self_playback_point <= _fsm_caller->last_applied_index() ||

relevant filesrc/braft/fsm_caller.cpp
suggestion      

Ensure thread safety for _last_applied_term updates in FSMCaller::set_self_playback_point. Since _last_applied_term is potentially accessed from multiple threads, consider using atomic operations or locks to prevent race conditions. [important]

relevant line_last_applied_term = _log_manager->get_term(self_playback_point);

relevant filesrc/braft/fsm_caller.cpp
suggestion      

Optimize the memory order used in _last_applied_index.store within FSMCaller::set_self_playback_point. Using butil::memory_order_relaxed might lead to issues in a multi-threaded environment where the order of writes is crucial. Consider using a stronger ordering like butil::memory_order_release. [medium]

relevant line_last_applied_index.store(self_playback_point,

relevant filesrc/braft/node.cpp
suggestion      

Add logging in NodeImpl::set_self_playback_point to provide visibility when playback points are set or when conditions prevent setting them. This can aid in monitoring and troubleshooting the system's behavior in production. [medium]

relevant lineif (self_playplayback_point <= _fsm_caller->last_applied_index() ||

@AlexStocks
Copy link

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add validation for non-negative playback points

Consider adding a check for self_playback_point being non-negative in
set_self_playplay_point method to ensure that the playback point is valid. This is crucial
as negative values could lead to undefined behavior or errors in log term retrieval.

src/braft/fsm_caller.cpp [552-556]

 void FSMCaller::set_self_playback_point(int64_t self_playback_point) {
+    if (self_playback_point < 0) {
+        return; // Optionally handle error or log this situation
+    }
     _last_applied_index.store(self_playback_point,
                               butil::memory_order_relaxed);
     _last_applied_term = _log_manager->get_term(self_playback_point);
 }
 
Suggestion importance[1-10]: 9

Why: Adding a check for non-negative values is crucial for preventing undefined behavior and potential errors. This suggestion addresses a possible bug and improves the robustness of the code.

9
Enhancement
Add error logging for invalid playback points

Implement error handling or logging when the self_playback_point is out of the valid range
in set_self_playback_point. This will help in debugging and ensure that the system behaves
predictably under erroneous conditions.

src/braft/node.cpp [970-976]

 void NodeImpl::set_self_playback_point(int64_t self_playback_point) {
     if (self_playback_point <= _fsm_caller->last_applied_index() || 
         self_playback_point > _log_manager->last_log_index()) {
+        LOG(ERROR) << "Invalid self_playback_point: " << self_playback_point;
         return;
     }
     _fsm_caller->set_self_playback_point(self_playback_point);
 }
 
Suggestion importance[1-10]: 8

Why: Adding error logging for invalid playback points enhances debugging and ensures predictable system behavior. This is a valuable enhancement for maintainability and reliability.

8
Best practice
Strengthen memory order for critical index updates

Use a stronger memory order for storing _last_applied_index in set_self_playback_point to
ensure visibility of changes across threads, considering the critical nature of log
indexing.

src/braft/fsm_caller.cpp [553]

 _last_applied_index.store(self_playback_point,
-                          butil::memory_order_relaxed);
+                          butil::memory_order_release);
 
Suggestion importance[1-10]: 7

Why: Strengthening the memory order to memory_order_release is a best practice for ensuring visibility of changes across threads, especially for critical updates like log indexing. This improves the reliability of the code.

7
Maintainability
Refactor conditional logic for clarity

Refactor the conditional logic in do_snapshot_load to improve readability and
maintainability. Extracting conditions into a well-named boolean variable can clarify the
intent.

src/braft/fsm_caller.cpp [437-440]

-if (last_applied_index() < meta.last_included_index()) {
+bool shouldUpdateIndex = last_applied_index() < meta.last_included_index();
+if (shouldUpdateIndex) {
     _last_applied_index.store(meta.last_included_index(),
                               butil::memory_order_release);
     _last_applied_term = meta.last_included_term();
 }
 
Suggestion importance[1-10]: 6

Why: Refactoring the conditional logic improves code readability and maintainability. While this is a good practice, it is not as critical as the other suggestions.

6

@AlexStocks AlexStocks merged commit d397128 into pikiwidb:master Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants