datafuselabs / openraft

rust raft with improvements
Apache License 2.0
1.36k stars 150 forks source link

normally there should be 2 config entries for the payload: max number of entries and max size in bytes for a payload. #181

Open drmingdrmer opened 2 years ago

drmingdrmer commented 2 years ago

Right normally there should be 2 config entries for the payload: max number of entries and max size in bytes for a payload.

Originally posted by @drmingdrmer in https://github.com/datafuselabs/openraft/issues/173#issuecomment-1042539657

github-actions[bot] commented 2 years ago

👋 Thanks for opening this issue!

Get help or engage by:

ppamorim commented 2 years ago

@drmingdrmer Remapping my suggestion to use a trait for that, could be more interesting this below?

trait Metrics {
  fn len(&self) -> u64;
}

struct Foo {
  value: String
}

pub enum StateMachineRequest {
  AddFoo { value: Foo },
  BatchAddFo { values_1: Vec<Foo>, values_2: Vec<Foo> },
}

impl Metrics for StateMachineRequest {
  fn len(&self) -> u64 {
    match self {
      StateMachineRequest2::AddFoo { value } => 1,
      StateMachineRequest2::BatchAddFo { values_1, values_2 } => {
        values_1.len() + values_2.len()
      }
    }
  }
}

If Foo does not implement Metrics, openraft could assume that size is 1 always.

drmingdrmer commented 2 years ago

I'm thinking about what you need is a batch client_write():

Raft.batch_client_write(reqs: Vec<StateMachineRequest>)

This way, a user does not need to implement the batching mechanism manually.

ppamorim commented 2 years ago

I believe that Batch is a very vague term for this case and does not represent in a concrete solution for the problem. The issue is envolved around the payload size, this can even be a issue if the system does not use batches andt the payload is big, this will cause the same issue. By doing that we are adding a compromise to the crate and users are expecting us to sort that out... IMO.

The state machine entry might have an arbritrary amount of data and it's the responsability of the server (not openraft) to either:

drmingdrmer commented 2 years ago

You are right. Let me fix the entry payload size limit issue first.