[jira] [Updated] (BOOKKEEPER-648) BasicJMSTest failed

[Mridul Muralidharan (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BOOKKEEPER-648?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Mridul Muralidharan updated BOOKKEEPER-648:
---

Assignee: (was: Mridul Muralidharan)

> BasicJMSTest failed
> ---
>
> Key: BOOKKEEPER-648
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-648
> Project: Bookkeeper
>  Issue Type: Bug
>  Components: hedwig-client
>Reporter: Flavio Junqueira
>
> While running tests, I got once a failure for this hedwig-client-jms test: 
> BasicJMSTest.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

[jira] [Updated] (BOOKKEEPER-560) Create readme for hedwig-client-jms

[Mridul Muralidharan (JIRA)]

 [ 
https://issues.apache.org/jira/browse/BOOKKEEPER-560?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Mridul Muralidharan updated BOOKKEEPER-560:
---

Assignee: (was: Mridul Muralidharan)

> Create readme for hedwig-client-jms 
> 
>
> Key: BOOKKEEPER-560
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-560
> Project: Bookkeeper
>  Issue Type: Task
>  Components: Documentation
>Reporter: Ivan Kelly
>
> This module needs a readme describing it as an experimental component and 
> what parts of jms are supported and not supported.
> It would also be good to have a bit more detail on why they can't be 
> supported with hedwig as it is today. A lot of this can be taken verbatim 
> from the package-info.html



--
This message was sent by Atlassian JIRA
(v6.2#6252)

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

[Sijie Guo]

> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 104
> > 
> >
> > You're sending StatusCode twice, every time. Firstly, shouldn't be an 
> > enum as stated above. Secondly, shouldn't be required.
> 
> Sijie Guo wrote:
> > enum
> 
> as the comments above.
> 
> > status code twice.
> 
> the status code for ReadResponse/AddResponse is for individual responses. 
> if we are going to support kind of batch reads, the status code of Response 
> means either the whole batch succeed or not while the individual read 
> responses might have different status codes (e.g OK or NotExists). That is 
> why it exists two places.
> 
> > required
> 
> as the comments above
> 
> Ivan Kelly wrote:
> Per request makes sense. However, in that case, can't you just remove the 
> per packet status?
> 
> Sijie Guo wrote:
> Nope. the packet status indicates whether the packet succeed or not. say 
> if it is batch read, if the total batch read failed, it would set the whole 
> packet status to be failed and there would be no individual responses in the 
> packet since the whole batch failed.
> 
> Ivan Kelly wrote:
> Then it should have a different name. Like entryStatus & packetStatus. 
> But then you also have a consistency problem. What is packetStatus is OK, but 
> one entryStatus is an error? Will there be a SOME_OK error? In what scenario 
> would a whole read batch fail?

if ledger isn't exists, then a packet status with NoSuchLedgerExists is 
returned. if ledger exists, but some entries are missing, so ok is returned for 
the packet, but NoSuchEntry would be returned for the missing entries.


> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80
> > 
> >
> > This would be better served with a boolean. protobuf will keep it 
> > compact.
> 
> Sijie Guo wrote:
> being Flag will make it easier to add other flags in future.
> 
> Ivan Kelly wrote:
> No it doesn't. Lets say you do add another flag in the future. How do you 
> specify that it's both a FENCE_LEDGER and NEW_FLAG? You need the field to be 
> repeated, and then you need code to loop through the flags. Compared to 
> msg.isFenceLedger(), this seems much more cumbersome.
> 
> Sijie Guo wrote:
> I don't say this field is bits-set-like of flags. as the long poll 
> changes we mentioned, we are going to add a new flag 'ENTRY_PIGGYBACK' for 
> read. so a read request is either a normal read (without Flag), a fence read 
> (flag = FENCE_LEDGER) or a piggyback read (flag = ENTRY_PIGGYBACK). Being 
> repeated will be kind of complicated, as you need to loop all possible 
> permutation.
> 
> Ivan Kelly wrote:
> This is ugly and shortsighted. There may well be a case in the future 
> where two flags need to be specified. What do we do then? Add a flags2 field?

why this couldn't be a 3rd flag? FENCE_LEDGER_AND_PIGGYBACK?

in your repeated way, let's say you have n flags, you would have n! ways to 
combine them. it is hard to figure out what kind of combination works for what 
situation. as some of your flags are exclusive with others. so renaming the 
combination specifically would make it clear.


> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 65
> > 
> >
> > Comment 1.
> > OperationType is redundant. The server can do #hasReadRequest and 
> > #hasAddRequest to check the request to check the type. We shouldn't send 
> > redundant data over the wire where possible.
> > 
> > Comment 2.
> > it better to make the absolute minimum to be required. Required is 
> > forever, and these may not be.
> > 
> > https://developers.google.com/protocol-buffers/docs/proto
> >
> 
> Sijie Guo wrote:
> > comment 1.
> 
> OperationType will make request more clear and specify and less if...else 
> branches. And OperationType is used for PCBC to unify all requests handling. 
> so we don't need to have individual maps to maintain requests each time we 
> added a new type of requests.
> 
> > comment 2.
> 
> if reply to comment 1 works for u, I don't see any reason why operation 
> would not be a required.
> 
> Ivan Kelly wrote:
> Ok. I can live with it, though I still think it's a little cumbersome. 
> Regarding requiredness, I still think it should be optional. All fields 
> should be optional by default to give you as much freedom to evolve the 
> protocol in the future as possible. Especially something, which is not 100% 
> essential to the protocol, like this field.
> 
> Sijie Guo wrote:
> for those fields that already exist in the origin

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

[Sijie Guo]

> On April 24, 2014, 12:19 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81
> > 
> >
> > ledgerId and entryId should be optional in all requests. It may be the 
> > case, that how we specify them changes in the future (like when we flatten 
> > the metadata), so it would be good to leave that possibility open.
> 
> Sijie Guo wrote:
> for all existing reads/writes protocol, the ledgerId and entryId are 
> required. I am not sure how u will change the protocol by flatten the 
> metadata. but I guess that will be pretty new protocol. if so, it would be 
> better to add new request type, so we will not break any existing systems or 
> making it being complicated.
> 
> Ivan Kelly wrote:
> Im not sure how it will change either. What I'm requesting is that the 
> protobuf protocol doesnt lock us in to how we are doing it now forever.
> 
> Ivan Kelly wrote:
> actually for a concrete example, lets say we want to do a read request 
> for a whole ledger, from bookie A we request all entries, but it doesnt have 
> every 3rd entry due to striping. In the case we can request all entrys from 
> the ledger with entryid modulo 3 from bookie B. In this case, what would I 
> put in the _required_ entry id firld for the read to bookie B?

as I said, doesn't it sound like a new read protocol?


- Sijie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review41281
---


On April 24, 2014, 7:43 a.m., Sijie Guo wrote:
> 
> ---
> 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 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/pom.xml ebc1198 
>   
> 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 
>   compat-deps/bookkeeper-server-compat-4.2.0/pom.xml PRE-CREATION 
>   compat-deps/hedwig-server-compat-4.2.0/pom.xml PRE-CREATION 
>   compat-deps/pom.xml f79582d 
>   hedwig-server/pom.xml 06cf01c 
>   
> hedwig-server/src/test/java/org/apache/hedwig/server/TestBackwardCompat.jav

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

[Ivan Kelly]

> On April 24, 2014, 12:19 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 81
> > 
> >
> > ledgerId and entryId should be optional in all requests. It may be the 
> > case, that how we specify them changes in the future (like when we flatten 
> > the metadata), so it would be good to leave that possibility open.
> 
> Sijie Guo wrote:
> for all existing reads/writes protocol, the ledgerId and entryId are 
> required. I am not sure how u will change the protocol by flatten the 
> metadata. but I guess that will be pretty new protocol. if so, it would be 
> better to add new request type, so we will not break any existing systems or 
> making it being complicated.

Im not sure how it will change either. What I'm requesting is that the protobuf 
protocol doesnt lock us in to how we are doing it now forever.


- Ivan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review41281
---


On April 24, 2014, 7:43 a.m., Sijie Guo wrote:
> 
> ---
> 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 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/pom.xml ebc1198 
>   
> 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 
>   compat-deps/bookkeeper-server-compat-4.2.0/pom.xml PRE-CREATION 
>   compat-deps/hedwig-server-compat-4.2.0/pom.xml PRE-CREATION 
>   compat-deps/pom.xml f79582d 
>   hedwig-server/pom.xml 06cf01c 
>   
> hedwig-server/src/test/java/org/apache/hedwig/server/TestBackwardCompat.java 
> 8da109e 
> 
> 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

[Ivan Kelly]

> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 33
> > 
> >
> > This should certainly not be an enum. Otherwise we need to bump the 
> > protocol version each time we add an error code.
> > 
> > Imagine the scenario where both server and client are running 4.3.0. 
> > Then the server is upgraded with 4.3.1 which a new error EPRINTERONFIRE. It 
> > sends this to the client who throws a decode error.
> 
> Sijie Guo wrote:
> how could being not enum help this? if it is integer, client still has no 
> idea how to interpret, so it is still invalid response from 4.3.0 client. I 
> thought we reached an agreement on enum on the ticket, no?
> 
> Ivan Kelly wrote:
> So for version and operationtype, enum is ok. These originate at the 
> client, so if the servers are always upgraded at the client, there's no 
> interoperability issues. Status codes originate at the server though, so it 
> is possible for the server to send a statuscode that is unrecognised to a 
> client. The normal way to handle this would be a "else" or "default:" to pass 
> this up to the client as a BKException.UnexpectedConditionException. If it's 
> an enum, this will throw a decode exception in the netty decoder, which is 
> harder to handle.
> 
> To resolve this on the server side, by checking the version and only 
> sending errors valid for that version, implies two things. Firstly, every 
> error code change will require the version to be bumped and secondly, that 
> there will need to be a list maintained for which errors are valid for each 
> version. This goes against the motivation for using protobuf in the first 
> place.
> 
> Sijie Guo wrote:
> this is the application level agreement, no? it doesn't matter that u are 
> using a protobuf protocol or using current protocol, or it also doesn't 
> matter that u are using an integer or an enum. in any case, the best way is 
> as you described, you shouldn't send new status code back to an old client, 
> as the new status code is meaningless to the old client.

but how do you know its an old client? Only by bumping the version number each 
time you add an error code. In which case you end up with a whole lot of junk 
like "if (client.version == X) { send A } else if (client.version == Y) { send 
B } else if (client.version ..." which is exactly what protobuf was designed to 
avoid (see "A bit of history" on 
https://developers.google.com/protocol-buffers/docs/overview).


> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 65
> > 
> >
> > Comment 1.
> > OperationType is redundant. The server can do #hasReadRequest and 
> > #hasAddRequest to check the request to check the type. We shouldn't send 
> > redundant data over the wire where possible.
> > 
> > Comment 2.
> > it better to make the absolute minimum to be required. Required is 
> > forever, and these may not be.
> > 
> > https://developers.google.com/protocol-buffers/docs/proto
> >
> 
> Sijie Guo wrote:
> > comment 1.
> 
> OperationType will make request more clear and specify and less if...else 
> branches. And OperationType is used for PCBC to unify all requests handling. 
> so we don't need to have individual maps to maintain requests each time we 
> added a new type of requests.
> 
> > comment 2.
> 
> if reply to comment 1 works for u, I don't see any reason why operation 
> would not be a required.
> 
> Ivan Kelly wrote:
> Ok. I can live with it, though I still think it's a little cumbersome. 
> Regarding requiredness, I still think it should be optional. All fields 
> should be optional by default to give you as much freedom to evolve the 
> protocol in the future as possible. Especially something, which is not 100% 
> essential to the protocol, like this field.
> 
> Sijie Guo wrote:
> for those fields that already exist in the origin protocol, I don't see 
> we are going to remove them. that's why we put it as required.

We shouldnt prelude the possibility though. What problem could making them 
optional cause? 


> On April 23, 2014, 10:22 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/proto/BookkeeperProtocol.proto, line 80
> > 
> >
> > This would be better served with a boolean. protobuf will keep it 
> > compact.
> 
> Sijie Guo wrote:
> being Flag will make it easier to add other flags in future.
> 
> Ivan Kelly wrote:
> No it doesn't. Lets say you do add another flag in the future. How do you 
> specify that it's both a FENCE_LEDGER and NEW_FLAG? You need the field to be 
> repeated, and then you need code to loop through the flags. Compared to 
> msg.isFe