[
https://issues.apache.org/jira/browse/BOOKKEEPER-582?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14017533#comment-14017533
]
Ivan Kelly commented on BOOKKEEPER-582:
---------------------------------------
My 2 main issues are (i.e. the source of the -1):
* *Use of an enum for flags:* This is covered a lot in review board, but
basically, by using an enum for the flag, you can only have one flag at a time.
Sijie's work around for this is to have combinatorial flags, but assuming each
flag is independent of each other flag, the number of entries for the enum
grows at O(n^2). In my opinion, the protocol would be cleaner and easier to
maintain if each flag was a boolean.
* *Sending error messages twice:* In the response part, there's an error
message for the packet and one for each individual response. According to
sijie, one is for packet level errors, the other is for entry level errors.
These should use separate error codes in this case. And it would be better if
these were only considered separate when there are multiple entries in a
response, which actually is never the case right now. For this, I would like to
see separate error enums (if we are stuck with using enums) for packet level
and entry level. Alternatively, we could remove the concept of packet error,
and have a container message for operations with multiple reads which can hold
the "packet" level error if needed.
If these two were resolved I would change my vote to -0 or +0. The other
issues, which are less important are:
* *Use of enum for errors:* Errors originate at the server, and the server can
have a newer version of the protocol than the client, so the server could
theoretically have a new error code that the client doesn't know about and this
would cause a runtime exception on the client. In my view it would be better to
have a catchall, so that you don't have to maintain a error compatibility map
on the server side which will push unknown errors into some catchall category
anyhow.
* *Too much required:* Again, this is an issue of leaving the protocol open to
future enhancements. We've had problems in hedwig that we couldn't change a
field for BC issues, so now we have to set it with a dummy value every time we
use the message even though neither the client nor the server use it.
Resolving these would move me to a +1.
> Make bookie and client use protobuf for requests (non-wire part)
> ----------------------------------------------------------------
>
> Key: BOOKKEEPER-582
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-582
> Project: Bookkeeper
> Issue Type: Sub-task
> Components: bookkeeper-client, bookkeeper-server
> Reporter: Ivan Kelly
> Assignee: Aniruddha
> Fix For: 4.3.0
>
> Attachments:
> 0002-BOOKKEEPER-582-Make-bookie-and-client-use-protobuf-f.patch,
> 0002-BOOKKEEPER-582-Make-bookie-and-client-use-protobuf-f.patch,
> 0002-BOOKKEEPER-582-Make-bookie-and-client-use-protobuf-f.patch,
> 0003-BOOKKEEPER-582-Make-bookie-and-client-use-protobuf-f.patch,
> BOOKKEEPER-582.diff, BOOKKEEPER-582.v2.diff, BOOKKEEPER-582.v3.diff
>
>
> Make the client and the bookie use protobufs internally. This is the first
> step to using protobufs on the wire, but for the moment, BookieRequestHandler
> decodes the old wire protocol into the protobuf messages. Once this is in,
> enabling on the wire will be very simple, and the old manual serialization
> can be made "legacy" (still supported, but deprecated).
--
This message was sent by Atlassian JIRA
(v6.2#6252)
