-
Notifications
You must be signed in to change notification settings - Fork 57
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
Sequenced vectors with efficient replication #239
Open
csibbitt
wants to merge
1
commit into
daid:master
Choose a base branch
from
csibbitt:csibbitt/efficient_log_replication
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
#ifndef SEQVECTOR_H | ||
#define SEQVECTOR_H | ||
|
||
#include <vector> | ||
|
||
namespace sp { | ||
|
||
// A vector that tracks sequenced elements | ||
template<typename T> class SeqVector : public std::vector<T>{ | ||
public: | ||
unsigned int last_seq = 0; | ||
}; | ||
|
||
}//namespace sp | ||
|
||
#endif//SEQVECTOR_H |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process of sending shouldn't modify the vector state. The concept of this code is that the
isChanged
call will check if it is changed and update data a theprev_data_ptr
to know there is no change anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you in principle ("The process of sending shouldn't modify the vector state"). We need somewhere to track what has been sent so that we don't resend the entire vector (say, of a sequence of log entries) every time we sync. A class that wraps the vector (whose state doesn't change) seemed like the right place. I experimented with just using a struct with a vector member and a last_seq member for clearer separation, but this was a cleaner fit for the templating. Maybe I've overlooked a simpler way.
I understand the prev_data approach of keeping a copy and comparing them, and started that way originally since that follows the pattern of the rest of the code. In addition to needlessly storing a second copy, I didn't come up with an easy solution to new client sync. Basically, instead of last_seq, I just used
prev_data_ptr->back()->seq
in it's place. The isChanged code wasif (ptr->back().seq != prev_data_ptr->back().seq)
, and the send code wasto_send = ... (ptr->back().seq - prev_data_ptr->back().seq)...
and they worked fine, but of course would not sync the backlog to a new client connection. I agree that the last_seq-shuffle in sendWholeDataSeqVector is a bit hacky, but I think it's pretty clear at least?This solution reduces memory footprint and makes for an easy way to sync new clients. The vector itself doesn't get modified by sending, just the super-class that is specifically FOR tracking. If there is a clearer way of expressing this within the framework we have here, I'm happy to learn it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't think it has to be a huge issue to send the whole log each time it changes (high logs/sec would be unreadable no mater the log size, so not really a realistic scenario). The main issue I see with the
master
code is that it's CPU heavy if you increase the log size. Keeping track of the sequence number inside theprev_data
would solve that part.