-
Notifications
You must be signed in to change notification settings - Fork 506
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
feat: add consumer storage to spu #3915
Conversation
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.
first set of comments
It would be better also break this into smaller PR |
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.
more comments
crates/fluvio-spu/src/services/internal/fetch_consumer_handler.rs
Outdated
Show resolved
Hide resolved
crates/fluvio-spu/src/services/internal/fetch_consumer_request.rs
Outdated
Show resolved
Hide resolved
Should separate out SPUSchema related changes into it's PR so can start merging that |
Ok(ReplicaRevStream::new(self.replica.clone())) | ||
} | ||
|
||
async fn append_batch(&mut self, entries: Vec<Vec<u8>>) -> Result<()> { |
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.
Can we crate some abstraction for for key value which I assume this is for. Just adding bytes seems to be bit raw
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 think we have enough abstractions here. Not sure I understand your questions.
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.
Try to add more description which hope to clarify purpose
crates/fluvio-spu/src/services/internal/update_consumer_offset_handler.rs
Show resolved
Hide resolved
Created #3918 |
TTL has been removed |
|
||
let consumers_replica_id = | ||
ReplicaKey::new(CONSUMER_STORAGE_TOPIC, <PartitionId as Default>::default()); | ||
|
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.
add more description on what's being done here
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.
Couple of minor clarification but no show stopper. LGTM to proceed
Added SPU private API (between SPUs) and public (except fetch list, will be added later) for Offset Mng.