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.

Sign up for GitHub

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

Jump to bottom

BP-25: MovingChecksumToProto--Refactor the checksum part of bookkeeper #1008

Closed
wants to merge 2 commits into from
Closed

BP-25: MovingChecksumToProto--Refactor the checksum part of bookkeeper #1008

wants to merge 2 commits into from

Conversation

ArvinDevel
Copy link
Contributor

@ArvinDevel ArvinDevel commented Jan 18, 2018 •
edited by sijie
Loading

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

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue # or BOOKKEEPER-#>: Description of pull request
    e.g. Issue 123: Description ...
    e.g. BOOKKEEPER-1234: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue # or BOOKKEEPER-#> in the title with the actual Issue/JIRA number.

@ArvinDevel
Copy link
Contributor Author

latest google doc: https://docs.google.com/document/d/1j1uq85dhqXvIcEucx1aEHJJS8mtOZAn4jVxnKTOhSys/edit?usp=sharing

sijie
sijie approved these changes Jan 18, 2018
View reviewed changes
Copy link
Member

@sijie sijie left a 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.

site/bps/BP-25-MovingChecksumToProto.md

### Proposed Changes

Move the DigestManager and related classes to proto module
Copy link
Member

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"?

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good Idea; +1. ;

Copy link
Contributor

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)

Copy link
Member

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.

@sijie sijie added this to the 4.7.0 milestone Jan 18, 2018
@ArvinDevel
Copy link
Contributor Author

@eolivelli I found that you created new api package, which includes DigestType. See #506. What should I consider If I combine the old DigestType in BookKeeper with it and move it to proto module? Sorry for that I don't understand the reason you create DigestType in api.

@ArvinDevel
Copy link
Contributor Author

the initial impl in #1018

@sijie
Copy link
Member

sijie commented Jan 23, 2018

retest this please

@sijie
Copy link
Member

sijie commented Jan 23, 2018

This BP is lazy approved with no -1.

sijie
sijie reviewed Jan 23, 2018
View reviewed changes
site/bps/BP-25-MovingChecksumToProto.md Outdated
---
title: "BP-25: MovingChecksumToProto--Refactor the checksum part of bookkeeper"
issue: https://github.com/apache/bookkeeper/issues/1007
state: "Under Discussion"
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@sijie sijie mentioned this pull request Jan 24, 2018
Open
@sijie sijie closed this in a512b73 Jan 24, 2018
@ArvinDevel ArvinDevel mentioned this pull request Jan 24, 2018
Closed
@sijie sijie mentioned this pull request Jan 24, 2018
Closed
@ArvinDevel ArvinDevel deleted the BP_25_MovingChecksumToProto branch January 25, 2018 01:52
sijie added a commit that referenced this pull request Jan 25, 2018
@sijie
Mark BP-20/BP-25 as adopted
4a4276f 
Descriptions of the changes in this PR:

BP-20 is adopted in #760  #761
BP-25 is adopted in #1007 #1008

This closes #760 #761 #1007 #1008

Author: Sijie Guo <sijie@apache.org>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Jia Zhai <None>

This closes #1047 from sijie/close_bp_25
@sijie sijie mentioned this pull request Jan 15, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvrao jvrao jvrao left review comments

@sijie sijie sijie approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

@ArvinDevel ArvinDevel

Labels
area/client area/protocol BP release/4.7.0 type/task
Projects
None yet
Milestone
4.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ArvinDevel @sijie @jvrao @eolivelli