You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/05/13 08:08:43 UTC
[GitHub] [geode] albertogpz opened a new pull request #6473: GEODE-9228: Fix possibly batches with incomplete transaction events s…
albertogpz opened a new pull request #6473:
URL: https://github.com/apache/geode/pull/6473
…ent when stopping gateway sender.
When stopping a gateway sender having group-transaction-events
set, there is a grace period in which events
received that belong to transactions are not dropped
if there are events in the queue belonging
to that transaction. This is done to make
sure that the queue does not contain incomplete
transactions.
Given that when the parallel gateway sender
creates a batch with events from the queue,
it removes them from the queue as it adds them
to the batch, it is possible that a transaction event
received by a gateway sender during
the stopping grace period
does not find events belonging to that transaction
in the queue because they have been added to
a current batch and therefore that event will
be dropped, causing that an incomplete transaction
will be finally stored in the queue/batch.
In order to solve the issue, when receiving a
transaction event during the stopping grace period,
the gateway sender must look for events belonging
to that transaction, not only in the queue
but also in the peekedEvents from the queue
(those events retrieved from the queue and
put in batches).
Also, in order to avoid flakyness of the distributed
test cases that test that batches with incomplete
transactions are not sent when stopping the
gateway sender, the number of retries
to get the events from the queue/batches
has been set to high number in the tests by means of
a new setter in GatewaySender which has
also been published as a gfsh and
cache.xml parameter (get-transaction-events-from-queue-retries).
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](http://www.apache.org/legal/resolved.html#category-a)?
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-845368576
> Is the "get-transaction-events-from-queue-retries" added only to help with the flakiness of tests (based on the description of the PR)? the xml attributes is user facing, should avoid adding very obscure meaning attributes if we can help it.
Yes. At first, I only added it to the GatewaySender interface to be able to set it in test cases to avoid flakyness. Then I thought I should also add it to the xml file and gfsh interfaces.
But we could just leave it in the GatewaySender interface and as a System property, as it already was.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-841513702
> > Can you fix the LGTM issue before I re-review this?
> > I believe I have already been through this once, so I want to minimize re-reviews...
> > Thanks!
>
> Done! Sorry about it.
No problem!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz edited a comment on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
albertogpz edited a comment on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-845368576
> Is the "get-transaction-events-from-queue-retries" added only to help with the flakiness of tests (based on the description of the PR)? the xml attributes is user facing, should avoid adding very obscure meaning attributes if we can help it.
@jinmeiliao Yes. At first, I only added it to the GatewaySender interface to be able to set it in test cases to avoid flakyness. Then I thought I should also add it to the xml file and gfsh interfaces.
But we could just leave it in the GatewaySender interface and as a System property, as it already was.
I have pushed a new commit without these parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #6473: GEODE-9228: Fix possibly batches with incomplete transaction events s…
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-840435582
This pull request **introduces 1 alert** when merging a09ba0823563f607dbe82976878fab6da60f5886 into 35f1696ab1fbd9d4aabb91eeabb7a7807eb394b8 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-e1e063153e45e4a607e340247a4cb497026c549f)
**new alerts:**
* 1 for Missing space in string literal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz closed pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
albertogpz closed pull request #6473:
URL: https://github.com/apache/geode/pull/6473
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-849524386
Kind reminder, code owners reviews still pending.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-845269282
Is the "get-transaction-events-from-queue-retries" added only to help with the flakiness of tests (based on the description of the PR)? the xml attributes is user facing, should avoid adding very obscure meaning attributes if we can help it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kirklund commented on a change in pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
kirklund commented on a change in pull request #6473:
URL: https://github.com/apache/geode/pull/6473#discussion_r650166221
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderEventProcessor.java
##########
@@ -136,14 +136,14 @@ protected boolean enqueueEvent(GatewayQueueEvent gatewayQueueEvent,
boolean queuedEvent = false;
try {
if (getSender().beforeEnqueue(gatewayQueueEvent)) {
+ if (condition != null &&
+ !((ParallelGatewaySenderQueue) this.queue).hasEventsMatching(
+ (GatewaySenderEventImpl) gatewayQueueEvent,
+ condition)) {
+ return false;
+ }
Review comment:
It might be best to some casts before the conditional just in order to improve readability:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue) this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching((GatewaySenderEventImpl) gatewayQueueEvent, condition)) {
return false;
}
```
We also need to consider updating interfaces like `RegionQueue` to avoid having to cast gatewayQueueEvent to GatewaySenderEventImpl. GatewaySenderEventImpl implements `InternalGatewayQueueEvent`. The interfaces start to lose value very fast if we let them become out-of-date. We should strive to use interfaces everywhere instead of concrete implementations.
Add:
```
boolean hasEventsMatching(GatewaySenderEventImpl event, Predicate<InternalGatewayQueueEvent> condition);
```
to `RegionQueue` (if that makes sense... I'm not an expert in Wan code).
Next change that method signature to:
```
boolean hasEventsMatching(InternalGatewayQueueEvent event, Predicate<InternalGatewayQueueEvent> condition);
```
You can start out by casting the arg `event` within the implementation of that method, but it's good to push that change through all of the code that `event` travels to. If any of those methods need implementation specifics, then update the interface `InternalGatewayQueueEvent`.
Then that conditional block becomes:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue) this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching(gatewayQueueEvent, condition)) {
return false;
}
```
You could even add `hasNoEventsMatching(...)` to ParallelGatewaySenderQueue.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderEventProcessor.java
##########
@@ -136,14 +136,14 @@ protected boolean enqueueEvent(GatewayQueueEvent gatewayQueueEvent,
boolean queuedEvent = false;
try {
if (getSender().beforeEnqueue(gatewayQueueEvent)) {
+ if (condition != null &&
+ !((ParallelGatewaySenderQueue) this.queue).hasEventsMatching(
+ (GatewaySenderEventImpl) gatewayQueueEvent,
+ condition)) {
+ return false;
+ }
Review comment:
It might be best to do some casts before conditionals just in order to improve readability. Example:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue) this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching((GatewaySenderEventImpl) gatewayQueueEvent, condition)) {
return false;
}
```
We also need to consider updating interfaces like `RegionQueue` to avoid having to cast gatewayQueueEvent to GatewaySenderEventImpl. GatewaySenderEventImpl implements `InternalGatewayQueueEvent`. The interfaces start to lose value very fast if we let them become out-of-date. We should strive to use interfaces everywhere instead of concrete implementations.
Add:
```
boolean hasEventsMatching(GatewaySenderEventImpl event, Predicate<InternalGatewayQueueEvent> condition);
```
to `RegionQueue` (if that makes sense... I'm not an expert in Wan code).
Next change that method signature to:
```
boolean hasEventsMatching(InternalGatewayQueueEvent event, Predicate<InternalGatewayQueueEvent> condition);
```
You can start out by casting the arg `event` within the implementation of that method, but it's good to push that change through all of the code that `event` travels to. If any of those methods need implementation specifics, then update the interface `InternalGatewayQueueEvent`.
Then that conditional block becomes:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue) this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching(gatewayQueueEvent, condition)) {
return false;
}
```
You could even add `hasNoEventsMatching(...)` to ParallelGatewaySenderQueue.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderEventProcessor.java
##########
@@ -136,14 +136,14 @@ protected boolean enqueueEvent(GatewayQueueEvent gatewayQueueEvent,
boolean queuedEvent = false;
try {
if (getSender().beforeEnqueue(gatewayQueueEvent)) {
+ if (condition != null &&
+ !((ParallelGatewaySenderQueue) this.queue).hasEventsMatching(
+ (GatewaySenderEventImpl) gatewayQueueEvent,
+ condition)) {
+ return false;
+ }
Review comment:
It might be best to do some casts before conditionals just in order to improve readability. Example:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue) this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching((GatewaySenderEventImpl) gatewayQueueEvent, condition)) {
return false;
}
```
We also need to consider updating interfaces like `RegionQueue` to avoid having to cast gatewayQueueEvent to GatewaySenderEventImpl. GatewaySenderEventImpl implements `InternalGatewayQueueEvent`. The interfaces start to lose value very fast if we let them become out-of-date. We should strive to use interfaces everywhere instead of concrete implementations.
Add:
```
boolean hasEventsMatching(GatewaySenderEventImpl event, Predicate<InternalGatewayQueueEvent> condition);
```
to `RegionQueue` (if that makes sense... I'm not an expert in Wan code).
Next change that method signature to:
```
boolean hasEventsMatching(InternalGatewayQueueEvent event, Predicate<InternalGatewayQueueEvent> condition);
```
You can start out by casting the arg `event` within the implementation of that method, but it's good to push that change through all of the code that `event` travels to. If any of those methods need implementation specifics, then update the interface `InternalGatewayQueueEvent`.
Then that conditional block becomes:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue) this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching(gatewayQueueEvent, condition)) {
return false;
}
```
The main point here is to always be updating the interfaces and don't let the implementation diverge so much that we need to use implementations instead of the interfaces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-841482870
> Can you fix the LGTM issue before I re-review this?
>
> I believe I have already been through this once, so I want to minimize re-reviews...
>
> Thanks!
Done! Sorry about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-841438653
Can you fix the LGTM issue before I re-review this?
I believe I have already been through this once, so I want to minimize re-reviews...
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #6473: GEODE-9228: Fix batches could be sent with incomplete transactions
Posted by GitBox <gi...@apache.org>.
albertogpz commented on pull request #6473:
URL: https://github.com/apache/geode/pull/6473#issuecomment-866036865
The original solution for GEODE-8971 is going to be reverted so this PR is not needed anymore given that it added the case for which a fix is provided in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org