GEODE-8302: Fixed 'events not queued conflated' stats when group-tran…

[albertogpz]

…saction-events is true

Batch conflation is not compatible with group-transaction-events.
It must be prevented that both are enabled at the same time for
a given gateway sender.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

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?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

[DonalEvans]

When the attached diff is applied, the modified test testPartitionedSerialPropagationHA() also fails with incorrectly conflated events. Since this test does not set groupTransactionEvents to true, it seems like there is more to this issue.
diff.txt

[DonalEvans]

Some additional description should also be added to the javadoc for the GatewaySenderFactory interface to state the incompatibility between the setGroupTransactionEvents() and setBatchConflationEnabled() settings. Other than that, this looks good to me.

[davebarnes97]

@albertogpz Good material - thanks for writing the docs for this feature.
I have some edits for clarification and consistency, in both user guide files and the javadoc comments in class files. I will send these to you in email for you to incorporate as you see fit. Thanks!

[albertogpz]

@albertogpz Good material - thanks for writing the docs for this feature.
I have some edits for clarification and consistency, in both user guide files and the javadoc comments in class files. I will send these to you in email for you to incorporate as you see fit. Thanks!

Thanks for the review, Dave. I have accepted all your proposals with the exception of the javadoc on GatewaySenderQueue.java which had not ended up describing correctly the peekedIdsWithoutExtra member variable. I changed it to a simpler sentence. Let me know if you are ok with the change.

[albertogpz]

In the last commit I reverted most of the code added in the second 4ff9b1d and just added a simpler fix.
The comment from @davebarnes97 about the javadoc in GatewaySenderQueue.java would not apply anymore as that javadoc would not change.

[davebarnes97]

Thank you for incorporating my previous suggestions. Just two more items:

In geode-docs/tools_modules/gfsh/command-pages/create.html.md.erb:
Please change "gatewaysenders" to "gateway senders".

In CreateGatewaySenderCommand.java and four other files, please reword the error message

"GatewaySender %s cannot be created with group transaction events set to true and batch conflation enabled"

to clarify that the two properties are mutually exclusive. Suggestion, add "both":

"GatewaySender %s cannot be created with both group transaction events set to true and batch conflation enabled"

[albertogpz]

Thank you for incorporating my previous suggestions. Just two more items:

In geode-docs/tools_modules/gfsh/command-pages/create.html.md.erb:
Please change "gatewaysenders" to "gateway senders".

In CreateGatewaySenderCommand.java and four other files, please reword the error message

"GatewaySender %s cannot be created with group transaction events set to true and batch conflation enabled"

to clarify that the two properties are mutually exclusive. Suggestion, add "both":

"GatewaySender %s cannot be created with both group transaction events set to true and batch conflation enabled"

Thanks for the comments, Dave.
Changes done on the erb file and on four java files. I hope you meant four files counting with CreateGatewaySenderCommand.java. I could not find the string in a fifth file.

[DonalEvans]

It appears that the failing StressNewTest seen in this PR is caused by https://issues.apache.org/jira/browse/GEODE-8320 and has not been introduced by these changes, so with a clean pre-checkin, this should be okay to merge, although ideally, it would be better to fix the underlying issue and then allow the StressNewTest to pass 100% of the time before merging this.

[albertogpz]

It appears that the failing StressNewTest seen in this PR is caused by https://issues.apache.org/jira/browse/GEODE-8320 and has not been introduced by these changes, so with a clean pre-checkin, this should be okay to merge, although ideally, it would be better to fix the underlying issue and then allow the StressNewTest to pass 100% of the time before merging this.

I'd rather have this PR merged first because fixing the flaky test case will probably take longer and I plan to do it on top of this solution.

[nabarunnag]

Hi Alberto, I would suggest splitting up the commit for GEODE-8320 fix separate from GEODE-8302.
You can still work on multiple commits for GEODE-8320

• This helps to maintain clear records in the release notes for releases.
• In case we have to revert the fix for GEODE-8320 we don't have to also revert the fix for GEODE-8302 and vice versa, as these two fixes are clubbed together

[albertogpz]

Hi Alberto, I would suggest splitting up the commit for GEODE-8320 fix separate from GEODE-8302.
You can still work on multiple commits for GEODE-8320

* This helps to maintain clear records in the release notes for releases.

* In case we have to revert the fix for GEODE-8320 we don't have to also revert the fix for GEODE-8302 and vice versa, as these two fixes are clubbed together

Hi Naba. What you are proposing makes a lot of sense. Nevertheless, in this case, I do not know if it is worth it as the solution for both tickets is shared, i.e., the solution for GEODE-8320 is a subset of the solution for GEODE-8302.
If I split the solution into two PRs I would have to repeat the solution on both PRs, have it reviewed twice, do changes if needed on both...
Do you still think I should do it like that or could we just have the PR for GEODE-8302 solve both GEODE-8302 and GEODE-8320?

[alb3rtobr]

@nabarunnag I agree that is not recommended to include more than one ticket solution in the same commit, but in this case one ticket is a subset of the other one. Luckily Alberto has solved the bigger one, that as a side effect solves the other. Or even you can think that it is invalidating the other one, as it cannot be reproduced after merging GEODE-8302.

Anyway independently of the decision taken, GEODE-8302 can be merged into develop if there is no other problem with the review. Then Alberto could prepare a separate PR with the code of GEODE-8320 that will result on an empty commit in develop because the code is already there.

[albertogpz]

It appears that the failing StressNewTest seen in this PR is caused by https://issues.apache.org/jira/browse/GEODE-8320 and has not been introduced by these changes, so with a clean pre-checkin, this should be okay to merge, although ideally, it would be better to fix the underlying issue and then allow the StressNewTest to pass 100% of the time before merging this.

I'd rather have this PR merged first because fixing the flaky test case will probably take longer and I plan to do it on top of this solution.

@DonalEvans Could you please review my latest commit? It was targeted at fixing a failing test case that was actually not the same as the one in GEODE-8320 but this last commit fixes the problem in both test cases.

[mkevo]

As it has two approvals I will merge it. Thank you all!