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

[ISSUE #124] add snapshot to raft state machine #127

Merged
merged 5 commits into from
Jul 26, 2022

Conversation

scutcr7
Copy link
Contributor

@scutcr7 scutcr7 commented Jul 21, 2022

@codecov-commenter
Copy link

Codecov Report

Merging #127 (ed56ff4) into develop (afd1302) will decrease coverage by 1.17%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #127      +/-   ##
===========================================
- Coverage    43.13%   41.95%   -1.18%     
===========================================
  Files          114      118       +4     
  Lines         4766     4900     +134     
  Branches       697      713      +16     
===========================================
  Hits          2056     2056              
- Misses        2356     2490     +134     
  Partials       354      354              
Impacted Files Coverage Δ
...ache/rocketmq/mqtt/meta/raft/MqttStateMachine.java 0.00% <0.00%> (ø)
...qtt/meta/raft/processor/CounterStateProcessor.java 0.00% <0.00%> (ø)
...ketmq/mqtt/meta/raft/processor/StateProcessor.java 0.00% <0.00%> (ø)
.../meta/raft/snapshot/AbstractSnapshotOperation.java 0.00% <0.00%> (ø)
...a/raft/snapshot/impl/CounterSnapshotOperation.java 0.00% <0.00%> (ø)
.../org/apache/rocketmq/mqtt/meta/util/DiskUtils.java 0.00% <0.00%> (ø)
...g/apache/rocketmq/mqtt/meta/util/RaftExecutor.java 0.00% <0.00%> (ø)

@@ -52,10 +57,13 @@ public class MqttStateMachine extends StateMachineAdapter {

private volatile String leaderIp = "unknown";

private SnapshotOperation snapshotOperation;
Copy link
Contributor

Choose a reason for hiding this comment

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

snapshotOperation is not used in this file, maybe it should be delete

public MqttStateMachine(MqttRaftServer server, StateProcessor processor, String groupId) {
this.server = server;
this.processor = processor;
this.groupId = groupId;
this.snapshotOperation = this.processor.loadSnapshotOperate();
Copy link
Contributor

Choose a reason for hiding this comment

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

This parameter is not used in this file


public abstract class StateProcessor {

public abstract Response onReadRequest(ReadRequest request);

public abstract Response onWriteRequest(WriteRequest log);

public SnapshotOperation loadSnapshotOperate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

onSnapshotSave/onSnapshotLoad is used to do Snapshot Operate.
SnapshotOperation maybe can be a implementation of onSnapshotSave/onSnapshotLoad, it can be delete

@@ -62,6 +72,24 @@ public Response onWriteRequest(WriteRequest writeRequest) {
}
}

@Override
public SnapshotOperation loadSnapshotOperate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

snapshotOperation can be init in constructor, or init function with @PostConstruct

Copy link
Contributor

Choose a reason for hiding this comment

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

However, you should consider that the user must use this framework for snapshot operations. This may be achieved through design such as inheritance

@pingww pingww merged commit ee3c4f6 into apache:develop Jul 26, 2022
@ShannonDing ShannonDing changed the title add snapshot to raft state machine [ISSUE #124] add snapshot to raft state machine Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants