[infinispan-dev] Stream encoding of Flags and future compatibility

[Dan Berindei]

On Tue, May 15, 2012 at 10:44 AM, Galder Zamarreño <galder at redhat.com> wrote:
>
> On May 11, 2012, at 11:30 PM, Dan Berindei wrote:
>
>> On Fri, May 11, 2012 at 7:23 PM, Sanne Grinovero <sanne at infinispan.org> wrote:
>>> On 11 May 2012 16:37, Galder Zamarreño <galder at redhat.com> wrote:
>>>> Quickly tried this and caused no issues:
>>>> https://github.com/galderz/infinispan/commit/7718926e5a4a6763506250362d7bd5cbdccd2931
>>>
>>> Looks good! I'm sure this doesn't solve all future migration problems,
>>> but if we could keep this kind of tricks around it should improve
>>> odds.
>>> IMHO, this is a kind of sensitivity that we should apply across all
>>> areas (not just flags).
>>>
>>
>> Looks interesting, but then you have the opposite problem: not all new
>> flags can be ignored, so you need a way to specify that a new flag is
>> "required". E.g. if we had just added a ZERO_LOCK_ACQUISITION_TIMEOUT
>> flag then the client would be expecting spurious failures, but not
>> extra long delays.
>
> Hmmm, I disagree. If you're adding a new flag, say in 5.2, and you expect a node that runs 5.0 to deal with it properly, really, what you need to be doing is implementing that flag in 5.0.
>

Well, 5.0 is already out there, so modifying it is not an option.
What we can do is ensure that the clients see the incompatibility in
their testing environment and don't use two versions in production
without being aware of the problem.

> We want:
> - if an old client encounters a new/unknown marshalled value, to not blow up and log a WARN.
> - if an old client is expected to react to a to a new/unknown marshalled value in the way new versions deal with it, it'll need to implement it.
>
> We don't want:
> - old client to 'blow up in flames' when they encounter new/unknown options, since this causes problems with potential rolling upgrades.
>

I think there are situations where two versions really are
incompatible and we really should "blow up in flames".
I'm not saying that's justified in all cases or even in the majority
of cases, but I'm pretty sure it's not going to be 0% either.

>>
>>> On a totally different page, why are we serializing Flags one-by-one ?
>>> We mostly need to serialize EnumSets right?
>>> An EnumSet can be encoded by using the bits of a couple of bytes.
>>> Three bytes looks like enough for all our needs.. we could even be
>>> clever and reserve a special Externalizer-ID for the empty set, to
>>> avoid 3 bytes where none are needed.
>>> While currently we need an integer (4 bytes) to encode the header for
>>> "EnumSet", plus (4 bytes header + 1 byte value) * each flag -> a lot.
>>>
>>
>> RiverMarshaller already has an optimization for the empty set:
>> https://github.com/dmlloyd/jboss-marshalling/blob/master/river/src/main/java/org/jboss/marshalling/river/RiverMarshaller.java#L613
>>
>> I'm not sure why it doesn't encode each element as a bit, it might be
>> to keep wire compatibility when the order of values in an enum
>> changes.
>
> David?
>
>> However, because there is only one EnumSet for all Enum types, a
>> hypothetical EnumSetExternalizer also needs to write the name of the
>> enum class - if we wanted to serialize EnumSet<Flag> in 2 bytes then
>> we'd need to make the transformation in ReplicableCommandExternalizer.
>
> Not necessarily. I think we should do what Sanne suggests but manually in the Flag.Externalizer class, since that's tied to the Flag type.
>
> Within it, we can replicate what an enum set does for marshalling. We already have such code in the Hot Rod server/client (that's how we handle flags there - completely forgot about it when I wrote Flag.Externalizer), so shouldn't be a biggie.
>

The call stack looks like this: ReplicableCommandExternalizer ->
EnumSet externalizer (in RiverMarshaller) -> Flag.Externalizer.
You can't change how the EnumSet is serialized in Flag.Externalizer,
you have to modify either the EnumSet externalizer (e.g. by writing a
new FlagSet class) or ReplicableCommandExternalizer.

The HotRod flags are serialized directly in Codec10.writeHeader, which
is the equivalent of ReplicableCommandExternalizer.writeCommandHeader.