You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jacob Barrett <ja...@vmware.com> on 2021/09/21 21:13:29 UTC

PROPOSAL: Remove WAN TX Batching Serialization Changes

Devs,

In addition to my discussion regarding the modularization of the WAN TX batching implementation I would like to propose that we remove the serialization changes that went into 1.14 to support it. Since the feature is not complete in 1.14 this should only impact the associated tests in 1.14. I want to do this to eliminate the necessary serialization of the of the transaction ID and last event flags as well as the boolean to determine if there is a transaction ID. As implemented right now this data is serialized for both WAN and AEQ sender events that are part of a transaction regardless of the enablement of TX batching on the sender. The transaction ID contains both the 4 byte counter and large membership ID.
https://github.com/apache/geode/blob/develop/geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java#L712

Since this went out in 1.14.0 the removal would be treated like any other upgrade to the protocol and a 1.14.1 version would not read or write any of those bites. When talking to exactly a 1.14.0 version the implementation would write only the false flag and read the flag and ignore the rest as necessary. The tests related to TX batching would also need to be disabled.

Something like this:

  public void toData(DataOutput out,
      SerializationContext context) throws IOException {
    // intentionally skip 1.14.0
    toDataPre_GEODE_1_14_0_0(out, context);
  }

  public void toDataPre_GEODE_1_14_1_0(DataOutput out,
      SerializationContext context) throws IOException {
    toDataPre_GEODE_1_14_0_0(out, context);
    DataSerializer.writeBoolean(false);
  }

  public void fromData(DataInput in, DeserializationContext context)
      throws IOException, ClassNotFoundException {
    fromDataPre_GEODE_1_14_1_0(in, context);
  }

  public void fromDataPre_GEODE_1_14_1_0(DataInput in, DeserializationContext context)
      throws IOException, ClassNotFoundException {
    fromDataPre_GEODE_1_14_0_0(in, context);
    if (version == KnownVersion.GEODE_1_14_0.ordinal()) {
      if (hasTransaction) {
        DataSerializer.readBoolean(DataSerializer.readBoolean(in));
        context.getDeserializer().readObject(in);
      }
    }
  }

I would also propose that if 1.15.0 looks like it will ship without the modularization changes that we at least address the serialization changes here in a way that does not affect all gateways, WAN or AEQ.

If accepted I will write up two JIRAs, one to address the 1.14 removal and the other as a blocker on 1.15 to address the serialization issues.

Ok, chime in!

-Jake

Re: PROPOSAL: Remove WAN TX Batching Serialization Changes

Posted by Jacob Barrett <ja...@vmware.com>.
For closure here is the PR:
https://github.com/apache/geode/pull/6892


On Sep 22, 2021, at 10:48 AM, Alberto Gomez <al...@est.tech>> wrote:

No worries, Jake.

The steps you are proposing sound good to me. Let me know if I can be of any help.

Alberto
________________________________
From: Jacob Barrett <ja...@vmware.com>>
Sent: Wednesday, September 22, 2021 7:44 PM
To: dev@geode.apache.org<ma...@geode.apache.org> <de...@geode.apache.org>>
Subject: Re: PROPOSAL: Remove WAN TX Batching Serialization Changes



On Sep 22, 2021, at 12:31 AM, Alberto Gomez <al...@est.tech>> wrote:

Hi,

Jake, why do you say the feature is not complete in 1.14.0? In my view, it works as it was designed and as documented.

Sorry, I was misinformed and misjudge the state. I meant no disrespect.

I do not think it makes sense to remove a feature shipped in the 1.14.0 release that customers might already be using.

I agree!

If the urgent issue to solve is the unnecessary serialization overhead when the WAN or AEQ events are part of a transaction and the sender has not enabled TX batching, I propose we try to fix just this in 1.14.1.

Yes, let me look at an alternative approach for 1.14 that can reduce that overhead when the sender is not batching transactions.

I think it makes send to scratch the original proposal. I will just write up a JIRA and PR for the protocol changes agains develop and then we can move them back to 1.14.1.

-Jake

Re: PROPOSAL: Remove WAN TX Batching Serialization Changes

Posted by Alberto Gomez <al...@est.tech>.
No worries, Jake.

The steps you are proposing sound good to me. Let me know if I can be of any help.

Alberto
________________________________
From: Jacob Barrett <ja...@vmware.com>
Sent: Wednesday, September 22, 2021 7:44 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: PROPOSAL: Remove WAN TX Batching Serialization Changes



> On Sep 22, 2021, at 12:31 AM, Alberto Gomez <al...@est.tech> wrote:
>
> Hi,
>
> Jake, why do you say the feature is not complete in 1.14.0? In my view, it works as it was designed and as documented.

Sorry, I was misinformed and misjudge the state. I meant no disrespect.

> I do not think it makes sense to remove a feature shipped in the 1.14.0 release that customers might already be using.

I agree!

> If the urgent issue to solve is the unnecessary serialization overhead when the WAN or AEQ events are part of a transaction and the sender has not enabled TX batching, I propose we try to fix just this in 1.14.1.

Yes, let me look at an alternative approach for 1.14 that can reduce that overhead when the sender is not batching transactions.

I think it makes send to scratch the original proposal. I will just write up a JIRA and PR for the protocol changes agains develop and then we can move them back to 1.14.1.

-Jake

Re: PROPOSAL: Remove WAN TX Batching Serialization Changes

Posted by Jacob Barrett <ja...@vmware.com>.

> On Sep 22, 2021, at 12:31 AM, Alberto Gomez <al...@est.tech> wrote:
> 
> Hi,
> 
> Jake, why do you say the feature is not complete in 1.14.0? In my view, it works as it was designed and as documented.

Sorry, I was misinformed and misjudge the state. I meant no disrespect.

> I do not think it makes sense to remove a feature shipped in the 1.14.0 release that customers might already be using.

I agree!

> If the urgent issue to solve is the unnecessary serialization overhead when the WAN or AEQ events are part of a transaction and the sender has not enabled TX batching, I propose we try to fix just this in 1.14.1.

Yes, let me look at an alternative approach for 1.14 that can reduce that overhead when the sender is not batching transactions.

I think it makes send to scratch the original proposal. I will just write up a JIRA and PR for the protocol changes agains develop and then we can move them back to 1.14.1.

-Jake

Re: PROPOSAL: Remove WAN TX Batching Serialization Changes

Posted by Alberto Gomez <al...@est.tech>.
Hi,

Jake, why do you say the feature is not complete in 1.14.0? In my view, it works as it was designed and as documented.

I do not think it makes sense to remove a feature shipped in the 1.14.0 release that customers might already be using.

If the urgent issue to solve is the unnecessary serialization overhead when the WAN or AEQ events are part of a transaction and the sender has not enabled TX batching, I propose we try to fix just this in 1.14.1.

Alberto

________________________________
From: Jacob Barrett <ja...@vmware.com>
Sent: Tuesday, September 21, 2021 11:13 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: PROPOSAL: Remove WAN TX Batching Serialization Changes

Devs,

In addition to my discussion regarding the modularization of the WAN TX batching implementation I would like to propose that we remove the serialization changes that went into 1.14 to support it. Since the feature is not complete in 1.14 this should only impact the associated tests in 1.14. I want to do this to eliminate the necessary serialization of the of the transaction ID and last event flags as well as the boolean to determine if there is a transaction ID. As implemented right now this data is serialized for both WAN and AEQ sender events that are part of a transaction regardless of the enablement of TX batching on the sender. The transaction ID contains both the 4 byte counter and large membership ID.
https://github.com/apache/geode/blob/develop/geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java#L712

Since this went out in 1.14.0 the removal would be treated like any other upgrade to the protocol and a 1.14.1 version would not read or write any of those bites. When talking to exactly a 1.14.0 version the implementation would write only the false flag and read the flag and ignore the rest as necessary. The tests related to TX batching would also need to be disabled.

Something like this:

  public void toData(DataOutput out,
      SerializationContext context) throws IOException {
    // intentionally skip 1.14.0
    toDataPre_GEODE_1_14_0_0(out, context);
  }

  public void toDataPre_GEODE_1_14_1_0(DataOutput out,
      SerializationContext context) throws IOException {
    toDataPre_GEODE_1_14_0_0(out, context);
    DataSerializer.writeBoolean(false);
  }

  public void fromData(DataInput in, DeserializationContext context)
      throws IOException, ClassNotFoundException {
    fromDataPre_GEODE_1_14_1_0(in, context);
  }

  public void fromDataPre_GEODE_1_14_1_0(DataInput in, DeserializationContext context)
      throws IOException, ClassNotFoundException {
    fromDataPre_GEODE_1_14_0_0(in, context);
    if (version == KnownVersion.GEODE_1_14_0.ordinal()) {
      if (hasTransaction) {
        DataSerializer.readBoolean(DataSerializer.readBoolean(in));
        context.getDeserializer().readObject(in);
      }
    }
  }

I would also propose that if 1.15.0 looks like it will ship without the modularization changes that we at least address the serialization changes here in a way that does not affect all gateways, WAN or AEQ.

If accepted I will write up two JIRAs, one to address the 1.14 removal and the other as a blocker on 1.15 to address the serialization issues.

Ok, chime in!

-Jake

Re: PROPOSAL: Remove WAN TX Batching Serialization Changes

Posted by Jacob Barrett <ja...@vmware.com>.

> On Sep 21, 2021, at 2:32 PM, Anilkumar Gingade <ag...@vmware.com> wrote:
> Is the idea just creating the Jira tickets? It is not clear from here, if it will be owned and completed by 1.15.

I will create two tickets:
1) To address 1.14 and anyone can pick this up, but if nobody does I will.
2) To block 1.15 on either the decisions to move to the modular approach or to fix the serialization issues by themselves in 1.15. Think of this is a placeholder for a decisions that needs to be made before 1.15 ships this serialization code again.

-Jake

Re: PROPOSAL: Remove WAN TX Batching Serialization Changes

Posted by Anilkumar Gingade <ag...@vmware.com>.
+1.
Is the idea just creating the Jira tickets? It is not clear from here, if it will be owned and completed by 1.15.

-Anil.


On 9/21/21, 2:13 PM, "Jacob Barrett" <ja...@vmware.com> wrote:

    Devs,

    In addition to my discussion regarding the modularization of the WAN TX batching implementation I would like to propose that we remove the serialization changes that went into 1.14 to support it. Since the feature is not complete in 1.14 this should only impact the associated tests in 1.14. I want to do this to eliminate the necessary serialization of the of the transaction ID and last event flags as well as the boolean to determine if there is a transaction ID. As implemented right now this data is serialized for both WAN and AEQ sender events that are part of a transaction regardless of the enablement of TX batching on the sender. The transaction ID contains both the 4 byte counter and large membership ID.
    https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fblob%2Fdevelop%2Fgeode-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgeode%2Finternal%2Fcache%2Fwan%2FGatewaySenderEventImpl.java%23L712&amp;data=04%7C01%7Cagingade%40vmware.com%7Ce671f8dae742430818a208d97d44b068%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637678556236321549%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=D6VAaF1JV8jRz7ggbwMn0nB9W8x5Xug6dAvOyGxJnKY%3D&amp;reserved=0

    Since this went out in 1.14.0 the removal would be treated like any other upgrade to the protocol and a 1.14.1 version would not read or write any of those bites. When talking to exactly a 1.14.0 version the implementation would write only the false flag and read the flag and ignore the rest as necessary. The tests related to TX batching would also need to be disabled.

    Something like this:

      public void toData(DataOutput out,
          SerializationContext context) throws IOException {
        // intentionally skip 1.14.0
        toDataPre_GEODE_1_14_0_0(out, context);
      }

      public void toDataPre_GEODE_1_14_1_0(DataOutput out,
          SerializationContext context) throws IOException {
        toDataPre_GEODE_1_14_0_0(out, context);
        DataSerializer.writeBoolean(false);
      }

      public void fromData(DataInput in, DeserializationContext context)
          throws IOException, ClassNotFoundException {
        fromDataPre_GEODE_1_14_1_0(in, context);
      }

      public void fromDataPre_GEODE_1_14_1_0(DataInput in, DeserializationContext context)
          throws IOException, ClassNotFoundException {
        fromDataPre_GEODE_1_14_0_0(in, context);
        if (version == KnownVersion.GEODE_1_14_0.ordinal()) {
          if (hasTransaction) {
            DataSerializer.readBoolean(DataSerializer.readBoolean(in));
            context.getDeserializer().readObject(in);
          }
        }
      }

    I would also propose that if 1.15.0 looks like it will ship without the modularization changes that we at least address the serialization changes here in a way that does not affect all gateways, WAN or AEQ.

    If accepted I will write up two JIRAs, one to address the 1.14 removal and the other as a blocker on 1.15 to address the serialization issues.

    Ok, chime in!

    -Jake