bookkeeper
bookkeeper copied to clipboard
apache
Add API for Bookie checksum verification writeFlags
Descriptions of the changes in this PR:
Motivation
Reserving the bits from WriteFlag so that clients can send the checksum information to the bookies
Changes
Updated WriteFlag class
Bit no 2-4 (from the right) indicates checksum type.
0 --> No verification enabled
1 --> DIGEST_TYPE_CRC32
2 --> DIGEST_TYPE_MAC
3 -->DIGEST_TYPE_CRC32C
Bit no 0 (from the right) is currently being used for DEFERRED_SYNC option.
Master Issue: #2174
Why do we have to define more constants, one for each supported digest type? The writer already knows the current digest type.
How would server know the digestType if we just have a single enum to represent them all. We would then need bit manipulation logic at other places to decode those bits to determine the type. This is helping consolidate some of it.
Side note: we cannot merge this change until the work on the bookie side is not ready and committed. We can release a bookie with some unused features but once we commit to the public client API the feature must be available on the server side.
That is fine, I wanted to put this PR out to get some feedback and also reserve the bits from writeFlags.
On the client API we should only have one WriteFlag CHECKSUM_VERIFICATION, as the LedgerHandle already knows the digest type.
On the wire we can still send more detail to the bookie, still using the 'flags' field
Another question: MAC needs a password, how will you handle it? We could say that MAC is not supported.
On the client API we should only have one WriteFlag CHECKSUM_VERIFICATION, as the LedgerHandle already knows the digest type.
I guess that comment is coming based on the fact that the WriteFlag enum is based in org.apache.bookkeeper.client.api package. However the server also uses it from the same place. DEFERRED_SYNC flag has been implemented the same way. The server needs to know the information of digestType and I don't think that parsing raw integer value outside of this enum is a good idea.
That being said, I wouldn't ask the app developer to add the corresponding flag on their own. Rather, it would get added automatically as part of LedgerHandle. Does that seem fine?
Another question: MAC needs a password, how will you handle it? We could say that MAC is not supported.
We are sending ledger key as part of the request too, hence we can use it.
@eolivelli Any comments here? I don't want to merge this now, just get an informal approval.
the MAC part is okay.
Still I think we must have only one enum value on the WriteFlags enum. We can change the server side and do use that class directly, we are free to decouple the wire protocol handling from the API.
So if I understand this correctly, we should copy over a writeFlag class to server side, have only 2 flags at the client level.
Since the server uses getWriteFlags() method to get a list of flags, we would still need those enum types on the server that map to the exact digest types. Another option is that I split over this method, and create separate methods that return different portions of write flag, boolean in case of DEFERRED_SYNC and an integer for digestType. Is that something you would like to do?
Sorry for late reply, I missed the notification.
Yes, on the client API we need only two flags.
For the server I like option 2, that is to create two methods: one for DEFERRED_SYNC and one for getting the DigestType
@eolivelli Currently I am not working, busy with some internal stuff. Will update the PR when I get some time.
@eolivelli Apologies for late reply.
I tried re-factoring the code but it's seems like a bigger effort. We ended up implementing the code internally in the same way as this PR.
I hope you will be able to port back your change to Apache Repo, we had a time with several forks and it was hard to merge them to a common form
Are you fine with the approach I have in this PR? @eolivelli
If yes, I can bring the other patches in too. If we want to modify the public API for WriteFlags, I am afraid that is a bigger work and I wont be able to take it up. Your approach is definitely cleaner, I agree on that.
@karanmehta93 I would like to see a better API for the client. I am ok on the wire protocol side.
We should see opinions for others. I could accept this patch and let you finish your port but then I would like to clean up the final API before a new release, I can help if you want.
I don't want to see new diverging forks, especially about wire protocol
@sijie @fpj @ivankelly @jvrao
