Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-29 Thread Sijie Guo
On Tue, May 27, 2014 at 11:52 PM, Rakesh R rake...@huawei.com wrote: On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80 This would be

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-29 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-29 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-28 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80 This would be better served with a boolean. protobuf will keep it compact. Sijie Guo

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-28 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-27 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-27 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80 This would be better served with a boolean. protobuf will keep it compact. Sijie Guo

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-26 Thread Sijie Guo
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review43940 --- just a ping to @fpj and @Rakesh - Sijie Guo On April 24, 2014,

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-21 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-20 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-20 Thread Rakesh R
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-20 Thread Sijie Guo
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-20 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-20 Thread Rakesh R
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-20 Thread Rakesh R
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-19 Thread Sijie Guo
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-19 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-15 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: Sijie Guo wrote: ok. let me step back. how bad is the current protocol? if no, why we keep arguing on this? as those changes will break our system, then I don't see the value of contributing our patches back to the community.

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-06 Thread Ivan Kelly
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80 This would be better served with a boolean. protobuf will keep it compact. Sijie Guo

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-05 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-05-04 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-29 Thread Ivan Kelly
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: Sijie Guo wrote: ok. let me step back. how bad is the current protocol? if no, why we keep arguing on this? as those changes will break our system, then I don't see the value of contributing our patches back to the community.

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-28 Thread Ivan Kelly
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: Sijie Guo wrote: ok. let me step back. how bad is the current protocol? if no, why we keep arguing on this? as those changes will break our system, then I don't see the value of contributing our patches back to the community. For

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-28 Thread Ivan Kelly
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-28 Thread Sijie Guo
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-28 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: Sijie Guo wrote: ok. let me step back. how bad is the current protocol? if no, why we keep arguing on this? as those changes will break our system, then I don't see the value of contributing our patches back to the community.

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-26 Thread Ivan Kelly
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80 This would be better served with a boolean. protobuf will keep it compact. Sijie Guo

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-25 Thread Ivan Kelly
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-25 Thread Sijie Guo
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-25 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 104 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line104 You're sending StatusCode twice, every time. Firstly, shouldn't be an enum as stated

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-24 Thread Sijie Guo
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/ --- (Updated April 24, 2014, 7:43 a.m.) Review request for bookkeeper and Ivan

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-24 Thread Ivan Kelly
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line80 This would be better served with a boolean. protobuf will keep it compact. Sijie Guo

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-24 Thread Ivan Kelly
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review41281 --- bookkeeper-server/src/main/proto/BookkeeperProtocol.proto

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-24 Thread Sijie Guo
On April 24, 2014, 12:19 p.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line81 ledgerId and entryId should be optional in all requests. It may be the case, that how we

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-24 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33 https://reviews.apache.org/r/17895/diff/2/?file=563033#file563033line33 This should certainly not be an enum. Otherwise we need to bump the protocol version each

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-23 Thread Ivan Kelly
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review41130 ---

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-04-23 Thread Sijie Guo
On April 23, 2014, 10:22 a.m., Ivan Kelly wrote: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java, line 131 https://reviews.apache.org/r/17895/diff/2/?file=563019#file563019line131 This change seems unrelated. I don't mind it going in, but could you

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-03-24 Thread Rakesh R
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/#review38282 --- Thanks Sijie, its really awsome! I just have few minor comments,

Re: Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-03-23 Thread Sijie Guo
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

Review Request 17895: BOOKKEEPER-582: protobuf support for bookkeeper

2014-02-09 Thread Sijie Guo
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17895/ --- Review request for bookkeeper and Ivan Kelly. Bugs: BOOKKEEPER-582