> 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 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?
>
> Sijie Guo wrote:
> 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.
>
> Ivan Kelly wrote:
> as you say, with n flags specified, there are n! combinations. so if
> another flag as added, you will have to add n values to the enum to cover the
> different combinations. whether the flags are exclusive is for the
> application to decide. it shouldnt be implicit in the protocol encoding.
>
> Sijie Guo wrote:
> same explanation for OperationType is applying here. if n flags mostly
> are used individually, an explicit way will just need to deal with around n
> possibilities. but you need to write lots of if..else branches for inferring
> in an implicit way.
>
> Ivan Kelly wrote:
> Your assumption here is that all the flag handling code will be in one
> place in a big switch. this isnt the case. I dont know how you have
> implemented piggyback, but i would guess it would be something like.
>
> Response processReadRequest(Request req) {
> if (isFencing(req)) {
> fenceLedger();
> }
> byte[] data = fetchEntry(req.getLedgerId(), req.getEntryId());
> Response resp = new Response(OK, data);
> if (isPiggyback(req)) {
> addPiggyback(resp);
> }
> return resp;
> }
>
> Now consider how #isPiggyback and #isFencing would be implemented with
> enums and with individual boolean flags? which is shorter? Which is easier to
> maintain?
>
> Sijie Guo wrote:
> isFencing => flag == Flag.FENCE_LEDGER
> isPiggyback => flag == ENTRY_PIGGYBACK
>
> what is the difference above for maintaining?
>
> using the flag, the semantic is super clear that your request is either a
> fencing request or a piggyback request. it would not happen that you received
> a request that probably have two flags enabled, then what does it mean if the
> requests have two boolean flags enabled, don't consider the case (that is
> always the point that I made for OperationType and Flag here)? then which is
> easier to maintain?
>
> Ivan Kelly wrote:
> my comment from yesterday doesn't wasnt saved by review board >:|
>
> Anyhow, what i asked was, can you guarantee that there will never *ever*
> be a case where two flags will need to be set on a single op in the future?
>
> Sijie Guo wrote:
> well, I already made a comment on your case: "why this couldn't be a 3rd
> flag? FENCE_LEDGER_AND_PIGGYBACK?", no? and this is how hedwig handles
> subscription mode, 'create', 'attach', 'create_and_attach', no?
>
> and again, as I said in previous comment, if you define flags in an
> explicit way, you would have much clearer branches on what kind of operations
> should be executed on what flags, like below: (take an example, A, B, C,
> A_AND_B, B_AND_A)
>
> switch (flag) {
> case A:
> a();
> break;
> case B:
> b();
> break;
> case C:
> c();
> break;
> case A_AND_B:
> a();
> b();
> break;
> case B_AND_A:
> b();
> // other executions between b() and a(), which is different from
> A_AND_B case.
> a();
> break;
> }
>
> but if you define flags in an implicit way, you need to check all
> possible negative cases to avoid the program failed in abnormal cases (e.g.
> bad combinations, bad order of flags). isn't that introducing much harder
> work for maintenance?
>
>
> Rakesh R wrote:
> If we have a set of flags, also consider both AND/OR conjunctions the
> combinations may grow and thus increases the complexity of the problem. I
> feel it doesn't matter whether you are using enum type or bool flag, both
> will have 'chain of if conditions' either at the client side or at the server
> side. If I understood correctly the switch case you are talking will be added
> at the server side. Now for setting 'A_AND_B' enum type in the request,
> client side we need to have the 'chain of if conditions' no?.
>
> I could see few advantages of having 'if chain' at the client side,
> server would not worry about the conditional evaluation which may be in a
> critical section. Secondly it would be easy to define strategies against enum
> type and pass it over different layers without worrying about how they formed.
>
> If we think the problem space is small and assume we have only few set of
> flags I prefer enum. Before reaching to a conclusion I've one question about
> the number 'n' of flags and how much value 'n' can grow ?. Only my worry is,
> once we set the protocol to enum type, as we all know couldn't get another
> chance to change it later as 'n' increases.
>
> Sijie Guo wrote:
> > Now for setting 'A_AND_B' enum type in the request, client side we need
> to have the 'chain of if conditions' no?.
>
> why client need to have the chain? client just need to set different flag
> value to indicate different type of single read.
>
> > how much value 'n' can grow?
>
> I could tell the future. but I don't see it would grow too much. A
> specific enum value would make semantic more clear.
I'm assuming there will be a conditional logic in order to set the flag and the
code will looks like,
Flag flag = // init with default value
if flag_condition_a satisfies
flag = A
if flag_condition_b satisfies
flag = B
if flag_condition_a satisfies && flag_condition_b satisfies
flag = A_AND_B
- 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
>
>
