Hi,

I'm woundering: The check operation is threaded like a write operation in
PrepRequestProcessor. It gets a TxnHeader attached and increments the zxid.
However the switch cases in ObserverRequestProcessor, CommitProcessor,
FollowerRequestProcessor and ReadOnlyRequestProcessor thread a check like a
read operation.
I don't know, whether this is a bug, but the inconsistency makes one
suspicious and merits an explaining comment.

Btw. The switch block in the ReadOnlyRequestProcessor does not list multi as a
write operation. Can this cause problems?

Best regards,

Thomas Koch, http://www.koch.ro

Both of these sound like bugs.

Sent from my iPhone

On Oct 20, 2011, at 6:44, Thomas Koch <[EMAIL PROTECTED]> wrote:

> Hi,
>
> I'm woundering: The check operation is threaded like a write operation in
> PrepRequestProcessor. It gets a TxnHeader attached and increments the zxid.
> However the switch cases in ObserverRequestProcessor, CommitProcessor,
> FollowerRequestProcessor and ReadOnlyRequestProcessor thread a check like a
> read operation.
> I don't know, whether this is a bug, but the inconsistency makes one
> suspicious and merits an explaining comment.
>
> Btw. The switch block in the ReadOnlyRequestProcessor does not list multi as a
> write operation. Can this cause problems?
>
> Best regards,
>
> Thomas Koch, http://www.koch.ro

Ted Dunning:
> Both of these sound like bugs.

Thank you for the quick reply.

BTW: I found this out while converting ZooDefs.OpCode to an Enum. I'd be glad,
if somebody would have a (longer) comment to my mail: "Enum too slow?"
Using an enum for the OpCode I can really do some very nice cleanups.

Regards,

Thomas Koch, http://www.koch.ro

i think you found the bug because you reviewed that code to convert to
enum. is there something inherit to using enums that helped? i don't
think it is wise to use enum when many (most perhaps) of the accesses
need to be integers.

ben

On Thu, Oct 20, 2011 at 6:28 AM, Thomas Koch <[EMAIL PROTECTED]> wrote:
> Ted Dunning:
>> Both of these sound like bugs.
>
> Thank you for the quick reply.
>
> BTW: I found this out while converting ZooDefs.OpCode to an Enum. I'd be glad,
> if somebody would have a (longer) comment to my mail: "Enum too slow?"
> Using an enum for the OpCode I can really do some very nice cleanups.
>
> Regards,
>
> Thomas Koch, http://www.koch.ro
>

One thing that might help relative to enums is that we can put all of the
knowledge about what an operation is into the enum so that inconsistencies
and omissions might be easier to see.

Regarding whether to use an integer or an enum, I really thought that enums
weren't much different from boxed integers in any case.  We can put the
necessary integer codes into the enum so it isn't difficult to get the code.

It is barely conceivable that there is a measurable cost to the boxing, but
I would be very surprised if so.  Many of the accesses as integers are
switch statements which would work as well as enums (and better because it
would provide completeness checking).

Ben, do you have something specific in mind here?  Were you referring to the
boxing/unboxing cost?  Or the clarity of the code in question?  Or something
I didn't think of?
On Fri, Oct 21, 2011 at 3:32 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:

> i think you found the bug because you reviewed that code to convert to
> enum. is there something inherit to using enums that helped? i don't
> think it is wise to use enum when many (most perhaps) of the accesses
> need to be integers.
>
>

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

On Fri, Oct 21, 2011 at 4:13 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
> One thing that might help relative to enums is that we can put all of the
> knowledge about what an operation is into the enum so that inconsistencies
> and omissions might be easier to see.
>
> Regarding whether to use an integer or an enum, I really thought that enums
> weren't much different from boxed integers in any case.  We can put the
> necessary integer codes into the enum so it isn't difficult to get the code.
>
> It is barely conceivable that there is a measurable cost to the boxing, but
> I would be very surprised if so.  Many of the accesses as integers are
> switch statements which would work as well as enums (and better because it
> would provide completeness checking).
>
> Ben, do you have something specific in mind here?  Were you referring to the
> boxing/unboxing cost?  Or the clarity of the code in question?  Or something
> I didn't think of?
>
>
> On Fri, Oct 21, 2011 at 3:32 PM, Benjamin Reed <[EMAIL PROTECTED]> wrote:
>
>> i think you found the bug because you reviewed that code to convert to
>> enum. is there something inherit to using enums that helped? i don't
>> think it is wise to use enum when many (most perhaps) of the accesses
>> need to be integers.
>>
>>
>

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

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
>

I agree with Ted, these are both definitely bugs.

On Thu, Oct 20, 2011 at 6:52 AM, Ted Dunning <[EMAIL PROTECTED]> wrote:

> Both of these sound like bugs.
>
> Sent from my iPhone
>
> On Oct 20, 2011, at 6:44, Thomas Koch <[EMAIL PROTECTED]> wrote:
>
> > Hi,
> >
> > I'm woundering: The check operation is threaded like a write operation in
> > PrepRequestProcessor. It gets a TxnHeader attached and increments the
> zxid.
> > However the switch cases in ObserverRequestProcessor, CommitProcessor,
> > FollowerRequestProcessor and ReadOnlyRequestProcessor thread a check like
> a
> > read operation.
> > I don't know, whether this is a bug, but the inconsistency makes one
> > suspicious and merits an explaining comment.
> >
> > Btw. The switch block in the ReadOnlyRequestProcessor does not list multi
> as a
> > write operation. Can this cause problems?
> >
> > Best regards,
> >
> > Thomas Koch, http://www.koch.ro
>

Did anyone open a Jira case on these bugs?

On Thu, Oct 20, 2011 at 10:07 AM, Marshall McMullen <
[EMAIL PROTECTED]> wrote:

> I agree with Ted, these are both definitely bugs.
>
> On Thu, Oct 20, 2011 at 6:52 AM, Ted Dunning <[EMAIL PROTECTED]>wrote:
>
>> Both of these sound like bugs.
>>
>> Sent from my iPhone
>>
>> On Oct 20, 2011, at 6:44, Thomas Koch <[EMAIL PROTECTED]> wrote:
>>
>> > Hi,
>> >
>> > I'm woundering: The check operation is threaded like a write operation
>> in
>> > PrepRequestProcessor. It gets a TxnHeader attached and increments the
>> zxid.
>> > However the switch cases in ObserverRequestProcessor, CommitProcessor,
>> > FollowerRequestProcessor and ReadOnlyRequestProcessor thread a check
>> like a
>> > read operation.
>> > I don't know, whether this is a bug, but the inconsistency makes one
>> > suspicious and merits an explaining comment.
>> >
>> > Btw. The switch block in the ReadOnlyRequestProcessor does not list
>> multi as a
>> > write operation. Can this cause problems?
>> >
>> > Best regards,
>> >
>> > Thomas Koch, http://www.koch.ro
>>
>
>