You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by fp...@apache.org on 2014/03/19 11:49:13 UTC
Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review37545
-----------------------------------------------------------
Ship it!
This is a great, I just have a few minor points and clarifications below.
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
<https://reviews.apache.org/r/17895/#comment69348>
Just to confirm, this is an improvement unrelated to this issue, yes?
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
<https://reviews.apache.org/r/17895/#comment69349>
It is good to use the version number (PreV3, V3) to differentiate parts of the code that serve different versions. This is a bit messy, though, because the version alone doesn't say what change actually triggered the code split. I was wondering if it makes sense to use ProtoBuf or PB, instead of V3, just to make it a bit more clear what change it is referring to in version 3.
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java
<https://reviews.apache.org/r/17895/#comment69139>
I think we still need to add the license header, no?
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java
<https://reviews.apache.org/r/17895/#comment69345>
Should we create a jira for this todo?
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java
<https://reviews.apache.org/r/17895/#comment69346>
This comment is repeated from the previous file. Do we need a jira for this todo?
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java
<https://reviews.apache.org/r/17895/#comment69347>
"... running in read-only mode..."
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java
<https://reviews.apache.org/r/17895/#comment69350>
Just to confirm, are we relying on the fact that compatibility with 4.1.0 implies compatibility with 4.2.0? I'm basically trying to understand why we don't need to add a new test case to test backward compatibility.
- fpj
On Feb. 10, 2014, 7:11 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17895/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2014, 7:11 a.m.)
>
>
> Review request for bookkeeper and Ivan Kelly.
>
>
> Bugs: BOOKKEEPER-582
> https://issues.apache.org/jira/browse/BOOKKEEPER-582
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> - introducing protobuf support for bookkeeper
> - for server: introduce packet processor / EnDecoder for different protocol supports
> - for client: change PCBC to use protobuf to send requests
> - misc changes for protobuf support
>
> (bookie server is able for backward compatibility)
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexInMemPageMgr.java 56487aa
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java 962f3a3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 241fdbb
> bookkeeper-server/src/main/java/org/apache/bookkeeper/processor/RequestProcessor.java 241f369
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java 1154047
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestHandler.java b922a82
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java 8155b22
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBaseV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 0757f87
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/proto/BookkeeperProtocol.proto PRE-CREATION
> bookkeeper-server/src/main/resources/findbugsExclude.xml 8404e3f
> bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/TestProtoVersions.java 5fcc445
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java 3f8496f
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java bc05229
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java 8376b46
>
> Diff: https://reviews.apache.org/r/17895/diff/
>
>
> Testing
> -------
>
> unit tests. backward tests.
>
>
> Thanks,
>
> Sijie Guo
>
>
Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper
Posted by Sijie Guo <gu...@gmail.com>.
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > This is a great, I just have a few minor points and clarifications below.
Thanks Flavio for reviewing. Will address your comments.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review37545
-----------------------------------------------------------
On Feb. 10, 2014, 7:11 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17895/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2014, 7:11 a.m.)
>
>
> Review request for bookkeeper and Ivan Kelly.
>
>
> Bugs: BOOKKEEPER-582
> https://issues.apache.org/jira/browse/BOOKKEEPER-582
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> - introducing protobuf support for bookkeeper
> - for server: introduce packet processor / EnDecoder for different protocol supports
> - for client: change PCBC to use protobuf to send requests
> - misc changes for protobuf support
>
> (bookie server is able for backward compatibility)
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexInMemPageMgr.java 56487aa
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java 962f3a3
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java 241fdbb
> bookkeeper-server/src/main/java/org/apache/bookkeeper/processor/RequestProcessor.java 241f369
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java 1154047
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestHandler.java b922a82
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java 8155b22
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBaseV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 0757f87
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/proto/BookkeeperProtocol.proto PRE-CREATION
> bookkeeper-server/src/main/resources/findbugsExclude.xml 8404e3f
> bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/TestProtoVersions.java 5fcc445
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java 3f8496f
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java bc05229
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java 8376b46
>
> Diff: https://reviews.apache.org/r/17895/diff/
>
>
> Testing
> -------
>
> unit tests. backward tests.
>
>
> Thanks,
>
> Sijie Guo
>
>
Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper
Posted by Sijie Guo <gu...@gmail.com>.
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > This is a great, I just have a few minor points and clarifications below.
>
> Sijie Guo wrote:
> Thanks Flavio for reviewing. Will address your comments.
sorry for late response. comments inline
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java, line 131
> > <https://reviews.apache.org/r/17895/diff/1/?file=481707#file481707line131>
> >
> > Just to confirm, this is an improvement unrelated to this issue, yes?
yup. it is unrelated. but since I generated the patch by diffing our branch with trunk branch, so I didn't split them.
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java, line 41
> > <https://reviews.apache.org/r/17895/diff/1/?file=481710#file481710line41>
> >
> > It is good to use the version number (PreV3, V3) to differentiate parts of the code that serve different versions. This is a bit messy, though, because the version alone doesn't say what change actually triggered the code split. I was wondering if it makes sense to use ProtoBuf or PB, instead of V3, just to make it a bit more clear what change it is referring to in version 3.
could I have a separated task to do renaming?
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java, line 1
> > <https://reviews.apache.org/r/17895/diff/1/?file=481713#file481713line1>
> >
> > I think we still need to add the license header, no?
added
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java, line 69
> > <https://reviews.apache.org/r/17895/diff/1/?file=481718#file481718line69>
> >
> > This comment is repeated from the previous file. Do we need a jira for this todo?
created https://issues.apache.org/jira/browse/BOOKKEEPER-748
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java, line 51
> > <https://reviews.apache.org/r/17895/diff/1/?file=481719#file481719line51>
> >
> > "... running in read-only mode..."
fixed
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java, line 530
> > <https://reviews.apache.org/r/17895/diff/1/?file=481726#file481726line530>
> >
> > Just to confirm, are we relying on the fact that compatibility with 4.1.0 implies compatibility with 4.2.0? I'm basically trying to understand why we don't need to add a new test case to test backward compatibility.
there is no change on protocol between 4.1.0 and 4.2.0. so we could use 4.1.0 implies compatibility with 4.2.0. but if adding 4.2.0 would make it clear, I could create a jira for it.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review37545
-----------------------------------------------------------
On April 21, 2014, 5:58 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17895/
> -----------------------------------------------------------
>
> (Updated April 21, 2014, 5:58 a.m.)
>
>
> Review request for bookkeeper and Ivan Kelly.
>
>
> Bugs: BOOKKEEPER-582
> https://issues.apache.org/jira/browse/BOOKKEEPER-582
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> - introducing protobuf support for bookkeeper
> - for server: introduce packet processor / EnDecoder for different protocol supports
> - for client: change PCBC to use protobuf to send requests
> - misc changes for protobuf support
>
> (bookie server is able for backward compatibility)
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexInMemPageMgr.java 56487aa
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java 28e23d6
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java fb36b90
> bookkeeper-server/src/main/java/org/apache/bookkeeper/processor/RequestProcessor.java 241f369
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java 1154047
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestHandler.java b922a82
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java 8155b22
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBaseV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java a10f7d5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/proto/BookkeeperProtocol.proto PRE-CREATION
> bookkeeper-server/src/main/resources/findbugsExclude.xml 97a6156
> bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/TestProtoVersions.java 5fcc445
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java 3f8496f
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java bc05229
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java 8376b46
>
> Diff: https://reviews.apache.org/r/17895/diff/
>
>
> Testing
> -------
>
> unit tests. backward tests.
>
>
> Thanks,
>
> Sijie Guo
>
>
Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper
Posted by Sijie Guo <gu...@gmail.com>.
> On March 19, 2014, 10:49 a.m., fpj wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java, line 67
> > <https://reviews.apache.org/r/17895/diff/1/?file=481717#file481717line67>
> >
> > Should we create a jira for this todo?
created https://issues.apache.org/jira/browse/BOOKKEEPER-748
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review37545
-----------------------------------------------------------
On April 21, 2014, 5:58 a.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17895/
> -----------------------------------------------------------
>
> (Updated April 21, 2014, 5:58 a.m.)
>
>
> Review request for bookkeeper and Ivan Kelly.
>
>
> Bugs: BOOKKEEPER-582
> https://issues.apache.org/jira/browse/BOOKKEEPER-582
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> - introducing protobuf support for bookkeeper
> - for server: introduce packet processor / EnDecoder for different protocol supports
> - for client: change PCBC to use protobuf to send requests
> - misc changes for protobuf support
>
> (bookie server is able for backward compatibility)
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexInMemPageMgr.java 56487aa
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java 28e23d6
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java fb36b90
> bookkeeper-server/src/main/java/org/apache/bookkeeper/processor/RequestProcessor.java 241f369
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java 1154047
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestHandler.java b922a82
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java 8155b22
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookkeeperProtocol.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBaseV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java a10f7d5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessorV3.java PRE-CREATION
> bookkeeper-server/src/main/proto/BookkeeperProtocol.proto PRE-CREATION
> bookkeeper-server/src/main/resources/findbugsExclude.xml 97a6156
> bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/TestProtoVersions.java 5fcc445
> bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java 3f8496f
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieClientTest.java bc05229
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/TestBackwardCompat.java 8376b46
>
> Diff: https://reviews.apache.org/r/17895/diff/
>
>
> Testing
> -------
>
> unit tests. backward tests.
>
>
> Thanks,
>
> Sijie Guo
>
>