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

Attach addition context to every proposals #59

Merged
merged 4 commits into from
May 14, 2018
Merged

Attach addition context to every proposals #59

merged 4 commits into from
May 14, 2018

Conversation

overvenus
Copy link
Member

It is helpful for special proposals when we want to trace them.

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

rest LGTM

src/raw_node.rs Outdated
@@ -257,6 +257,11 @@ impl<T: Storage> RawNode<T> {

// Propose proposes data be appended to the raft log.
pub fn propose(&mut self, data: Vec<u8>, sync_log: bool) -> Result<()> {
self.propose_with_context(data, sync_log, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to remove sync_log from the propose signature, since it's part of the context, which should be handled by users.

@siddontang
Copy link
Contributor

can you give an example of how can we use this feature?

@BusyJay
Copy link
Member

BusyJay commented May 12, 2018

So we can add more flags to context without the cost of deserializing data field. This is a more generic solutions to flag sync-log and what #50 tries to add.

The final solution should be using lazy deserialization of protobuf directly, so that user can put any context to the data field instead of coupling with raft-rs's proto definition. But there seems no rust implementation yet.

@@ -12,6 +12,7 @@ message Entry {
uint64 index = 3;
bytes data = 4;
bool sync_log = 5;
uint64 context = 6;
Copy link
Member

Choose a reason for hiding this comment

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

The context type is uint64, which is very confused.

@overvenus
Copy link
Member Author

@siddontang A concrete example can be found at tikv/tikv#3045, split commands put a flag into the context, so we don't need to deserialize the data field to get types of commands.

@ngaut It is somewhat confusing, it could be bytes which compiles to Vec<u8>, but I think u64 is good enough to storage various context, besides u64 is cheaper than Vec<u8>.

@siddontang
Copy link
Contributor

@overvenus

u64 is not enough if we want to support more contexts later.

@siddontang
Copy link
Contributor

I prefer using bytes here, and we can use lazy decode in our own logic.

We can also remove sync-log flag.

@BusyJay
Copy link
Member

BusyJay commented May 14, 2018

Any updates?

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

rest LGTM

src/raw_node.rs Outdated
@@ -256,29 +256,27 @@ impl<T: Storage> RawNode<T> {
}

// Propose proposes data be appended to the raft log.
pub fn propose(&mut self, data: Vec<u8>, sync_log: bool) -> Result<()> {
pub fn propose(&mut self, data: Vec<u8>, context: Vec<u8>) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

context should go first.

@@ -12,6 +12,7 @@ message Entry {
uint64 index = 3;
bytes data = 4;
bool sync_log = 5;
Copy link
Contributor

@siddontang siddontang May 14, 2018

Choose a reason for hiding this comment

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

maybe we can add a comment to deprecate sync log here.

@siddontang
Copy link
Contributor

Rest LGTM

Copy link
Contributor

@siddontang siddontang left a comment

Choose a reason for hiding this comment

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

LGTM

@BusyJay
Copy link
Member

BusyJay commented May 14, 2018

LGTM

@BusyJay BusyJay merged commit a874d93 into master May 14, 2018
@BusyJay BusyJay deleted the context branch May 14, 2018 09:13
@Hoverbear Hoverbear added this to the 0.2.0 milestone Jul 5, 2018
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.

5 participants