-
Notifications
You must be signed in to change notification settings - Fork 903
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
BP-25: MovingChecksumToProto--Refactor the checksum part of bookkeeper #1008
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.
+1 on this idea. it will be good to have this shared between client and bookie.
at twitter, we used to have an implementation that validate checksum before storing entries to bookies. it was done in a bit hacky way (for addressing urgent network corruption issue). we were thinking about moving checksum stuff to a common module which can be shared between client and bookie, so we can do checksum validation properly. now someone is working on that. glad to see it happen.
|
||
### Proposed Changes | ||
|
||
Move the DigestManager and related classes to proto module |
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.
which package are you thinking of "org.apache.bookkeeper.proto" or "org.apache.bookkeeper.proto.checksum"?
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 "org.apache.bookkeeper.proto.checksum" is more clear, and the proto module already has lots of classes. What's your idea?
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.
Good Idea; +1. ;
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.
@sijie What was the performance penalty you paid for the check on the bookies? I believe we needed that to avoid corruption at the source (client nic)
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.
@ArvinDevel : +1 on org.apache.bookkeeper.prot.checksum
@jvrao I think it has slight increased cpu usage but I don't have an exact number of that, because at that time we don't have CRC32C. now we have a native CRC32C implementation, this should be much better now.
and yes, agreed with you (and I believed we chatted about this long time ago) we have to do the bookie side checksum validation before storing entries.
@eolivelli I found that you created new api package, which includes |
the initial impl in #1018 |
retest this please |
This BP is lazy approved with no -1. |
--- | ||
title: "BP-25: MovingChecksumToProto--Refactor the checksum part of bookkeeper" | ||
issue: https://github.com/apache/bookkeeper/issues/1007 | ||
state: "Under Discussion" |
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.
@ArvinDevel can you update this to "Accepted"? so I can merge this BP proposal.
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.
updated
4082a93
to
c76199d
Compare
Descriptions of the changes in this PR:
Current the checksum implementation is in client module while the checksum semantic is more close to protocol. Moreover, moving the checksum implementation to protocol will avoid server module's dependency to client module when doing checksum in server side.
Master Issue: #1007