Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Zookeeper >> mail # dev >> Why does multi-op check operation increment zxid?


Copy link to this message
-
Re: Why does multi-op check operation increment zxid?
to be honest it is lines like this

-               switch (hdr.getType()) {
-               case OpCode.createSession: {
+               switch (OpCode.fromInt(hdr.getType())) {
+               case createSession: {

and especially
-                        h.setType(ZooDefs.OpCode.setWatches);
+                        h.setType(OpCode.setWatches.getInt());

that ruin the enum for me. it's not that i don't like enums, it's just
that in this case we are using a serialization class that is
generating ints that sucks away a lot of the benefits. a field that
keeps switching between int and enum is just a bit weird and a little
confusing. i'd rather stick with consistency. we should, and do, use
enums if it is a field that is always used as a enum.

i actually started to write some of the things you are converting to
enums as enums originally, but i found the switching code just a
little too ugly, and i abandoned it.

i really don't feel strongly about this case, since i have thought
both ways about this particular field in the past. i do feel rather
strongly about a big patch for a rather non-issue. and your patch is
very big, and there is a bunch of other issues we really need to be
addressing. so, it's sucking up some valuable committer time.

having said that, if a committer does think it is a good idea and
wants to shepherd the patch, i certainly wouldn't stand in the way.

ben

On Mon, Oct 24, 2011 at 10:52 AM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> Benjamin Reed:
>> i was thinking mostly in terms of the code itself. enums are great for
>> structures that aren't serialized, but when you do serialize it, you
>> end up having to force specific values into the enum and converting
>> back and forth during the serialization and deserialization. i find
>> that the enums, in this case, just shift the ugliness around.
>>
>> i'm also not sure about the performance implication, it should just be
>> the cost of autoboxing, but to be honest it's more about the code
>> readability to me.
>>
>> ben
> Hi Ben,
>
> I've updated the patch for ZK-1199 (make OpCode an enum). Please see yourself,
> whether you consider the resulting code to be uglier or not. Especially in the
> following classes I could remove a couple of lines and complexity:
>
> PrepRequestProcessor, CommitProcessor, FollowerRequestProcessor,
> ObserverRequestProcessor, ReadOnlyRequestProcessor
>
> If you're concerned about readability, we should trust Joshua Bloch. One of
> the advantages pointed out by him is type safety. The Request class has long
> zxid, long sessionId, long cxid, and int type. During refactoring I've had a
> hard time to not mix positions of the constructor parameters. The IDE can not
> help me to distinguish between different number types.
>
> Best regards,
>
> Thomas Koch, http://www.koch.ro
>