> 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 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.
>
> Ivan Kelly wrote:
> 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).
>
> Sijie Guo wrote:
> a else or default branch would make the behavior unpredictable as an old
> client is treating a new status code as some kind of unknown. as you said,
> you want to treat them as UnexpectedConditionException. But what does
> UnexpectedConditionException means? doesn't it mean the sever already breaks
> backward compatibility, since server couldn't satisfy the old client's
> request.
>
> so still, if server wants to be backward compatibility to clients, in any
> cases, it needs to know what version of protocol that the client is speaking
> and handle them accordingly, not just let client to do their job in an
> unexpected way.
>
> I don't see any elegant solutions without detecting protocol version. if
> you have, please describe how not being enum would avoid this.
>
> Ivan Kelly wrote:
> the default behaviour for an unknown error code is something we already
> use today.
>
> https://github.com/apache/bookkeeper/blob/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L714
>
> The client only needs to know that the request failed. the point of the
> different error codes is so that the client could take specific recovery
> steps. the default behaviour is just to pass the error up.
>
> Sijie Guo wrote:
> the default behavior was there just for all already known status codes.
> but it doesn't mean it is correct for any unknown status codes. and when u
> are saying 'the client only needs to know that the request failed', you are
> making an assumption that there is only one status code indicating OK, other
> status code should be taken as failed. but it isn't true. say in an old
> protocol, we supported range reads, it responded with OK, list of entry
> response (0 = <data>, 1 = missing, 2 = missing, 3 = <data>). if we are going
> to improve our protocol to make communication more efficient, we are going to
> change the protocol to get rid of transferring missing entries: responding
> with PARTIAL_OK, list of existing entries (0 = <data>, 3 = <data>).
>
> in this case, if server doesn't distinguish the client's protocol, just
> respond every range reads with PARTIAL_OK, would did break the compatibility
> with old protocol, as old protocol treats it as failure by default behavior.
> in order to maintain backward compatibility, server needs to detect the
> client's protocol and responds accordingly. as what I said, for backward
> compatibility, a protocol doesn't really help if it involves behavior in
> application agreement. and a default behavior is not exact right.
>
> for me, using protocol. it means that it is easy for us to add new
> requests type, add new fields in existing requests. besides that, we need to
> keep the backward compatibility in our level.
>
> Ivan Kelly wrote:
> when i say that 'the client only needs to know that the request failed',
> it means that the client only needs to recognise the statuses which indicate
> success. for any request, the status codes which indicate success should
> never change. For a read request or add request, only OK is SUCCESS. For
> range read, only PARTIAL_OK and OK are success. But the client will never
> send a range read request unless it already knows about PARTIAL_OK.
>
> Sijie Guo wrote:
> > “But the client will never send a range read request unless it already
> knows about PARTIAL_OK.”
>
> the example that I made is that the range read introduced with OK, but if
> it is going to evolved to have PARTIAL_OK. then that will be an problem.
>
> Ivan Kelly wrote:
> Partial_ok doesn't make sense if you're using the concept of packet
> status and operation status (as mentioned below).
>
> Sijie Guo wrote:
> well, if u are talking about a specific case (e.g. range read), I might
> agree with u. but if you read my comments, it is just the case that I
> explained how the protocol might be evolved. And my point is and always will
> be for backward compatibility, the server shouldn't send any unknown
> codes/fields back to clients that speaks old protocol, this is part of
> application-level agreement, no matter using enum or integer.
>
> Rakesh R wrote:
> Using enum is more readable and would be good for newcomers too. Assume a
> case where we are introducing new EC for an existing request type at the side
> server side, EC#NEW_ERR_CODE in 4.3.1 and client is old one 4.3.0. Ideally in
> this case, bk client should say its unknown code.
>
> Does this create any parsing issues at the protobuf ?. If not, I'm OK
> using enum for the StatusCode too.
>
> Sijie Guo wrote:
> if you read the conversation about this, my point is that for correct
> backward compatibility, we should avoid sending unknown error codes back to
> an old client.
>
> Rakesh R wrote:
> So server should have version specific checks and generates the error
> codes, isn't it ?.
>
> Sijie Guo wrote:
> yes. exact as what I pointed. sending responses matching exact version of
> protocol, isn't that exactly for application level backward compatible? why
> this would be the concern?
>
> Rakesh R wrote:
> There is no big concern here. I'm trying to understand the current
> approach and the future path to avoid any compatibility issues later.
>
> Sijie Guo wrote:
> for backward compatibility, it is comprised of wire compatibility and
> application-semantic compatibility. using protobuf, we could easily achieve
> wire compatibility by adding new optional fields. for application-semantic,
> the server needs to detect what version of the client is talking, and send
> correct version of responses according to its version. the version field in
> the request/response structure helps achieving this.
>
> Rakesh R wrote:
> Maintaining version list has its own complexities, but again it depends
> on the client side requirements. As I know protobuf parser would not get
> decode exceptions if any unknown enum code comes from server, instead this
> will return a null value. If this is the behavior of protobuf, then today its
> not a blocker for this patch. Because in your patch, you have already handled
> unknown error code in PerChannelBookieClient#statusCodeToExceptionCode() and
> treats this code as an op failure.
>
> Sijie Guo wrote:
> version list is more for application level semantic backward
> compatibility. a clear specific version for backward compatibility would be
> more clear than any implicit solution. although it might introduce code, it
> is clear for maintenance.
OK. My point is, since we have already unknown error code handling at the
client side, we can discuss this case alone later. Today its not a concern for
this patch, but we can atleast remember this point for future discussion :-)
+ Integer rcToRet = statusCodeToExceptionCode(status);
+ if (null == rcToRet) {
+ rcToRet = BKException.Code.WriteException;
+ Integer rcToRet = statusCodeToExceptionCode(status);
+ if (null == rcToRet) {
+ rcToRet = BKException.Code.ReadException;
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17895/#review41130
-----------------------------------------------------------
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
>
>
