Going Distributed #19
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SUMUKHA-PK
reviewed
Aug 15, 2020
Contributor
SUMUKHA-PK
left a comment
SUMUKHA-PK
left a comment
There was a problem hiding this comment.
I'm not gonna review the core lockclient or lockservice yet as this is gonna be merged after my PR.
This is a first round of review just for code semantics, I'll have future rounds based on the correctness.
So far, good job!
| func (f *fsm) Apply(l *raft.Log) interface{} { | ||
| var c command | ||
| if err := json.Unmarshal(l.Data, &c); err != nil { | ||
| panic(fmt.Sprintf("failed to unmarshal command: %s", err.Error())) |
Contributor
There was a problem hiding this comment.
Can you not panic and return the error?
Panics stop the system function, we shouldn't allow that.
| return f.applyRelease(c.Key, c.Value) | ||
|
|
||
| default: | ||
| panic(fmt.Sprintf("unrecognized command op: %s", c.Op)) |
|
|
||
| // Snapshot returns a snapshot of the key-value store. We wrap | ||
| // the things we need in fsmSnapshot and then send that over to Persist. | ||
| // Persist encodes the needed data from fsmsnapshot and transport it to |
Contributor
There was a problem hiding this comment.
Suggested change
| // Persist encodes the needed data from fsmsnapshot and transport it to | |
| // Persist encodes the needed data from fsmsnapshot and transports it to |
| } | ||
|
|
||
| // Set the state from snapshot. No need to use mutex lock according | ||
| // to Hasicorp doc |
Contributor
There was a problem hiding this comment.
Suggested change
| // to Hasicorp doc | |
| // to Hasicorp doc. |
|
|
||
| func (f *fsmSnapshot) Persist(sink raft.SnapshotSink) error { | ||
| err := func() error { | ||
| // Encode data |
Contributor
There was a problem hiding this comment.
Suggested change
| // Encode data |
| // If a node already exists with either the joining node's ID or address, | ||
| // that node may need to be removed from the config first. | ||
| if srv.ID == raft.ServerID(nodeID) || srv.Address == raft.ServerAddress(addr) { | ||
| // However if *both* the ID and the address are the same, then nothing -- not even |
Contributor
There was a problem hiding this comment.
Suggested change
| // However if *both* the ID and the address are the same, then nothing -- not even | |
| // However if BOTH the ID and the address are the same, then nothing, not even |
| // that node may need to be removed from the config first. | ||
| if srv.ID == raft.ServerID(nodeID) || srv.Address == raft.ServerAddress(addr) { | ||
| // However if *both* the ID and the address are the same, then nothing -- not even | ||
| // a join operation -- is needed. |
Contributor
There was a problem hiding this comment.
Suggested change
| // a join operation -- is needed. | |
| // a join operation, is needed. |
| "github.com/gorilla/mux" | ||
| ) | ||
|
|
||
| // func (rs *lockservice.RaftStore) Start() error { |
| // lockservice's lock map. | ||
| // This function is used by the Snapshot() function of the | ||
| // finite state machine of the distributed consensus algorithm | ||
| func (ls *SimpleLockService) GetLockMap() map[string]string { |
Contributor
There was a problem hiding this comment.
You said copy, return the copy.
Here locking is of no use since you're returning the actual map anyway.
| // function | ||
| func (ls *SimpleLockService) SetLockMap(lockMapSnapshot map[string]string) { | ||
| // Set the state from snapshot. No need to use mutex lock according | ||
| // to Hasicorp doc |
Contributor
There was a problem hiding this comment.
Suggested change
| // to Hasicorp doc | |
| // to Hashicorp doc |
SUMUKHA-PK
changed the title
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #16
This pull request adds the ability to host
SimpleLockServiceon a cluster of nodes in consensus. Consensus is maintained using the Raft algorithm. The Hashicorp implementation of the algorithm has been used.A struct of type
RaftStorehas been created. It has the following fields -Each Raft node has an associated HTTP listener. If a request is sent to the listener of the leader of a cluster, then the listener commits the operation on the associated Raft node if possible. If a request is sent to the listener of a follower in the cluster, this request is redirected to the listener of the leader. The implementation is such that the listener has an IP address with a port number that is one more than the port number of the associated Raft node. (for example, if the IP address of a Raft node is 127.0.0.1:5000, then the IP address of its listener is 127.0.0.1:5001).
getHTTPAddr()andgetListenerAddr()functions implement this mapping.Redirection of HTTP requests to the listener of the leader node has been tested in
simple_client_test.goalong with the tests that are already present.