Re: [DISCUSS] Modularizing new WAN TX Batching (and Modularization Efforts in General)
I think this is a great initiative. Not only are you giving a reminder about the need to implement new features in a modular and pluggable way, but you are also providing a real example with an already implemented not in the best way feature, to show the steps to follow in the right direction for this and future features. I would be interested in attending to a live walkthrough over the details of the changes. Thanks, Alberto From: Jacob Barrett Sent: Tuesday, September 21, 2021 3:04 AM To: dev@geode.apache.org Subject: Re: [DISCUSS] Modularizing new WAN TX Batching (and Modularization Efforts in General) Unfortunately I can’t do meetings that early. The earliest I could do that day is 9am PDT. > On Sep 20, 2021, at 3:34 PM, Anthony Baker wrote: > > The approach makes sense to me. The idea of a stable core + extensibility is > a common and successful pattern in many OSS projects. > > @Jake, you want to discuss at the next Geode Community meeting in Oct? > > Anthony > > >> On Sep 20, 2021, at 1:48 PM, Jacob Barrett wrote: >> >> Devs, >> >> We need to be doing a better job with implementing new features in a modular >> and plugable way. We have had discussions in the past but we haven’t been >> the best at sticking to it. Most recently we began work on a modified >> version of WAN that supports transactional batching. Rather than implement >> it in a plugable model we modified the existing implementation with a new >> property that changes the internal behavior of the implementation. This is a >> clear smell that what we have should be plugable and modularized. I am not >> suggesting that we run out and define clear public SPIs for everything or >> come up with some complicated plan to re-architect everything into modules >> but rather that we take small steps. I will argue that when we are adding >> functionality to a core service that is the point we should consider steps >> to break it up into clear module components. Think to yourself, what would >> it take for me to implement this new functionality as its own module, >> meaning its own jar and Gradle sub-project. As you begin to develop the >> solution you may find you need some clean interfaces for it to extend from >> the core, that might be the start of an internal SPI. You may find that some >> concrete classes you would have normally modified just need to be extended >> with a few protected methods to implement alternative logic. >> >> I think we should focus efforts on extracting an interface to plug in >> different WAN gateway implementations so that existing implementations >> aren’t modified with new behavior. With proper interface extraction we can >> more easily unit test around WAN gateways. By keeping implementations small >> we can more easily test them in isolation. Making them all plugable allows >> distributions of Geode to deliver specific implementations they would like >> to support without impacting the existing implementations. It also frees >> Geode to release new experimental or beta implementations of WAN gateways >> without impacting the existing implementations rather than delaying releases >> waiting for modified WAN gateways to be production ready and fully tested. >> >> In looking at the geode-wan module one might notice that it was already >> designed to be plugable. Unfortunately it isn’t that easy. This SPI was >> originally there to provide a way for Geode to be donated initially without >> WAN support. It turns out that most of the core to WAN is actually still in >> geode-core and only some of the “secret sauce” was moved into geode-wan. The >> bulk of the geode-core code for WAN is actually in support of the region >> queues for WAN and AEQ, so moving it into geod-wan would have cut off AEQ. >> While it would be possible to utilize this SPI for providing alternative >> gateway implementations it is very high level,so much of the alternative >> implementations would be duplications, and it doesn’t allow for both >> implementations to sit side by side at runtime. I would actually suggest we >> eliminate this public SPI in favor of just the geode-wan core module that it >> is and eventually migrate the region queue code into its own module as well, >> but these are for another day. >> >> Looking closer at the WAN gateways themselves there is mostly a pluggable >> interface already there with the existing interfaces. I spent a little time >> pulling apart an internal SPI and it was quite easy. With a small >> modification to gfsh and cache xml to specify that alternative >> implementation by name is all that needs to be done immediately to configure >> an alternative. Without extracting too many of the common implementation out >> into its own module just a few of the classes in geode-core can be modified >> to provide empty implementation of key overridable plug-in points for the TX >> batching implementation. The result is
Re: [DISCUSS] Modularizing new WAN TX Batching (and Modularization Efforts in General)
I know we have folks spread across different time zones, would a 9AM PST / 4PM UTC discussion work? Yes, we will post the recording afterwards but that makes it hard to ask live questions :-) Anthony > On Sep 21, 2021, at 12:14 AM, Alberto Gomez wrote: > > I think this is a great initiative. Not only are you giving a reminder about > the need to implement new features in a modular and pluggable way, but you > are also providing a real example with an already implemented not in the best > way feature, to show the steps to follow in the right direction for this and > future features. > > I would be interested in attending to a live walkthrough over the details of > the changes. > > Thanks, > > Alberto > > From: Jacob Barrett > Sent: Tuesday, September 21, 2021 3:04 AM > To: dev@geode.apache.org > Subject: Re: [DISCUSS] Modularizing new WAN TX Batching (and Modularization > Efforts in General) > > Unfortunately I can’t do meetings that early. The earliest I could do that > day is 9am PDT. > >> On Sep 20, 2021, at 3:34 PM, Anthony Baker wrote: >> >> The approach makes sense to me. The idea of a stable core + extensibility >> is a common and successful pattern in many OSS projects. >> >> @Jake, you want to discuss at the next Geode Community meeting in Oct? >> >> Anthony >> >> >>> On Sep 20, 2021, at 1:48 PM, Jacob Barrett wrote: >>> >>> Devs, >>> >>> We need to be doing a better job with implementing new features in a >>> modular and plugable way. We have had discussions in the past but we >>> haven’t been the best at sticking to it. Most recently we began work on a >>> modified version of WAN that supports transactional batching. Rather than >>> implement it in a plugable model we modified the existing implementation >>> with a new property that changes the internal behavior of the >>> implementation. This is a clear smell that what we have should be plugable >>> and modularized. I am not suggesting that we run out and define clear >>> public SPIs for everything or come up with some complicated plan to >>> re-architect everything into modules but rather that we take small steps. I >>> will argue that when we are adding functionality to a core service that is >>> the point we should consider steps to break it up into clear module >>> components. Think to yourself, what would it take for me to implement this >>> new functionality as its own module, meaning its own jar and Gradle >>> sub-project. As you begin to develop the solution you may find you need >>> some clean interfaces for it to extend from the core, that might be the >>> start of an internal SPI. You may find that some concrete classes you would >>> have normally modified just need to be extended with a few protected >>> methods to implement alternative logic. >>> >>> I think we should focus efforts on extracting an interface to plug in >>> different WAN gateway implementations so that existing implementations >>> aren’t modified with new behavior. With proper interface extraction we can >>> more easily unit test around WAN gateways. By keeping implementations small >>> we can more easily test them in isolation. Making them all plugable allows >>> distributions of Geode to deliver specific implementations they would like >>> to support without impacting the existing implementations. It also frees >>> Geode to release new experimental or beta implementations of WAN gateways >>> without impacting the existing implementations rather than delaying >>> releases waiting for modified WAN gateways to be production ready and fully >>> tested. >>> >>> In looking at the geode-wan module one might notice that it was already >>> designed to be plugable. Unfortunately it isn’t that easy. This SPI was >>> originally there to provide a way for Geode to be donated initially without >>> WAN support. It turns out that most of the core to WAN is actually still in >>> geode-core and only some of the “secret sauce” was moved into geode-wan. >>> The bulk of the geode-core code for WAN is actually in support of the >>> region queues for WAN and AEQ, so moving it into geod-wan would have cut >>> off AEQ. While it would be possible to utilize this SPI for providing >>> alternative gateway implementations it is very high level,so much of the >>> alternative implementations would be duplications, and it doesn’t allow for >>> both implementations to sit side by side at runtime. I would actually >>> suggest we eliminate this public SPI in favor of just the geode-wan core >>> module that it is and eventually migrate the region queue code into its own >>> module as well, but these are for another day. >>> >>> Looking closer at the WAN gateways themselves there is mostly a pluggable >>> interface already there with the existing interfaces. I spent a little time >>> pulling apart an internal SPI and it was quite easy. With a small >>> modification to gfsh and cache xml to
Re: [DISCUSS] Modularizing new WAN TX Batching (and Modularization Efforts in General)
Shoot I have a personal appointment that day I can’t move at that time. We could schedule a special session for this or move the community meeting to a different day. I could do Monday or Tuesday that week at 9:00 am PDT. > On Sep 21, 2021, at 8:14 AM, Anthony Baker wrote: > > I know we have folks spread across different time zones, would a 9AM PST / > 4PM UTC discussion work? Yes, we will post the recording afterwards but that > makes it hard to ask live questions :-) > > Anthony > > >> On Sep 21, 2021, at 12:14 AM, Alberto Gomez wrote: >> >> I think this is a great initiative. Not only are you giving a reminder about >> the need to implement new features in a modular and pluggable way, but you >> are also providing a real example with an already implemented not in the >> best way feature, to show the steps to follow in the right direction for >> this and future features. >> >> I would be interested in attending to a live walkthrough over the details of >> the changes. >> >> Thanks, >> >> Alberto >> >> From: Jacob Barrett >> Sent: Tuesday, September 21, 2021 3:04 AM >> To: dev@geode.apache.org >> Subject: Re: [DISCUSS] Modularizing new WAN TX Batching (and Modularization >> Efforts in General) >> >> Unfortunately I can’t do meetings that early. The earliest I could do that >> day is 9am PDT. >> On Sep 20, 2021, at 3:34 PM, Anthony Baker wrote: >>> >>> The approach makes sense to me. The idea of a stable core + extensibility >>> is a common and successful pattern in many OSS projects. >>> >>> @Jake, you want to discuss at the next Geode Community meeting in Oct? >>> >>> Anthony >>> >>> On Sep 20, 2021, at 1:48 PM, Jacob Barrett wrote: Devs, We need to be doing a better job with implementing new features in a modular and plugable way. We have had discussions in the past but we haven’t been the best at sticking to it. Most recently we began work on a modified version of WAN that supports transactional batching. Rather than implement it in a plugable model we modified the existing implementation with a new property that changes the internal behavior of the implementation. This is a clear smell that what we have should be plugable and modularized. I am not suggesting that we run out and define clear public SPIs for everything or come up with some complicated plan to re-architect everything into modules but rather that we take small steps. I will argue that when we are adding functionality to a core service that is the point we should consider steps to break it up into clear module components. Think to yourself, what would it take for me to implement this new functionality as its own module, meaning its own jar and Gradle sub-project. As you begin to develop the solution you may find you need some clean interfaces for it to extend from the core, that might be the start of an internal SPI. You may find that some concrete classes you would have normally modified just need to be extended with a few protected methods to implement alternative logic. I think we should focus efforts on extracting an interface to plug in different WAN gateway implementations so that existing implementations aren’t modified with new behavior. With proper interface extraction we can more easily unit test around WAN gateways. By keeping implementations small we can more easily test them in isolation. Making them all plugable allows distributions of Geode to deliver specific implementations they would like to support without impacting the existing implementations. It also frees Geode to release new experimental or beta implementations of WAN gateways without impacting the existing implementations rather than delaying releases waiting for modified WAN gateways to be production ready and fully tested. In looking at the geode-wan module one might notice that it was already designed to be plugable. Unfortunately it isn’t that easy. This SPI was originally there to provide a way for Geode to be donated initially without WAN support. It turns out that most of the core to WAN is actually still in geode-core and only some of the “secret sauce” was moved into geode-wan. The bulk of the geode-core code for WAN is actually in support of the region queues for WAN and AEQ, so moving it into geod-wan would have cut off AEQ. While it would be possible to utilize this SPI for providing alternative gateway implementations it is very high level,so much of the alternative implementations would be duplications, and it doesn’t allow for both implementations to sit side by side at runtime. I would actually suggest we eliminate this public SPI in favor of just the geode-wan core module that it is and even
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
+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" 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&data=04%7C01%7Cagingade%40vmware.com%7Ce671f8dae742430818a208d97d44b068%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637678556236321549%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=D6VAaF1JV8jRz7ggbwMn0nB9W8x5Xug6dAvOyGxJnKY%3D&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
Re: PROPOSAL: Remove WAN TX Batching Serialization Changes
> On Sep 21, 2021, at 2:32 PM, Anilkumar Gingade 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
