geode icon indicating copy to clipboard operation
geode copied to clipboard

Wip/wan tx grouping module 1

Open albertogpz opened this issue 3 years ago • 6 comments

For all changes:

  • [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • [ ] Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • [ ] Is your initial contribution a single, squashed commit?

  • [ ] Does gradlew build run cleanly?

  • [ ] Have you written or updated unit tests to verify your changes?

  • [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

albertogpz avatar Nov 05 '21 08:11 albertogpz

@albertogpz Can you please rebase this from develop. There as a fixed added there to address issues with Gradle builds.

jake-at-work avatar Nov 30 '21 20:11 jake-at-work

@albertogpz we need to extract out the TX batching specific elements form the GatewaySenderEventImpl object. I replaced it with a implementation defined metadata object. See TxBatchMetadata from my previous example on making this a module. This gets set on the event object when constructed via an override on AbstractGatewaySenderEventProcessor. See example here.

jake-at-work avatar Dec 01 '21 00:12 jake-at-work

Inlined a few comments. Overall looks like a good direction! Here are some general comments and tips:

1. Prefer interfaces over abstract methods.

2. Standardize on one library instead of mixing them (such as AssertJ for assertions).

3. Is there a better word than "txgrouping" for module name and packages?

4. Declare variables and APIs with interfaces rather than implementations.

5. Always look for an interface to use instead of an implementation (such as InternalCache instead of GemFireCacheImpl) in both tests and product code.

6. Always minimize the scope of anything (class, method, variable, constant).

Thanks for your comments. Regarding the name, I do not have one that I really like. Other alternatives could be: txeventsgrouping, grouptxevents, txaware,... If you like any of these or have any other proposal, please, let me know.

albertogpz avatar Dec 02 '21 05:12 albertogpz

@albertogpz we need to extract out the TX batching specific elements form the GatewaySenderEventImpl object. I replaced it with a implementation defined metadata object. See TxBatchMetadata from my previous example on making this a module. This gets set on the event object when constructed via an override on AbstractGatewaySenderEventProcessor. See example here.

@pivotal-jbarrett As I said in a previous e-mail, I do not think we can make these changes without breaking backward compatibility. The feature was production ready at 1.14 so we can expect that it is used by customers. Therefore, we cannot just ignore those changes when interworking with 1.14 servers. Am I missing something?

albertogpz avatar Dec 02 '21 15:12 albertogpz

this PR appears to be abandoned, can it be closed?

onichols-pivotal avatar Feb 05 '22 08:02 onichols-pivotal

this PR appears to be abandoned, can it be closed?

It is not abandoned. Still waiting for comments after review changes. I have rebased the branch to remove the conflicts with develop.

albertogpz avatar Feb 07 '22 08:02 albertogpz