albertogpz commented on pull request #6659:
URL: https://github.com/apache/geode/pull/6659#issuecomment-872768866
> > > @pivotal-eshu
> > > It looks like the Tx code is trying to assess Gateway sender
requirement to support GEODE-7971 in TX code. Which doesn't seems right.
> > > The "getLastTransactionEvent" should just return the last event in the
transaction; currently it is doing it by "callbacks.get(callbacks.size() - 1);"
which also seems to be error prone or inefficient.
> > > In "firePendingCallbacks" it could have iterated over the list using
size index:
> > > for (int i=0; i<callbaack.size(); i++){
> > > ee.getRegion().invokeTXCallbacks(EnumListenerEvent.AFTER_DESTROY, ee,
true, (i+1) == size() /_isLastTrasaction_/);
> > > }
> > > Please correct me if i am missing anything.
> >
> >
> > It was implemented that way to put the logic of getting the last
transaction event in another class although it is as simple as getting the last
element.
> > Also, this logic could detect misconfigurations that can only be
detected at runtime (second check in `getLastTransactionEvent()`), then the
caller of getLastTransactionEvent() could detect it by means of the exception
and set all the events as the last in the transaction. Why do this? because if
not all events in the transaction go to the same senders (misconfiguration),
that would imply that some senders will not get all the events for the
transaction but could try if group-transaction-events is set to complete the
transaction in the batch, by looking for the last event in the transaction
which could never come (delaying the batch sending) or may come but not
guaranteeing that the rest of the events for the transaction were sent in the
batch (because they may not have reached the sender).
> > The first check of `getLastTransactionEvent()` was added in order to
avoid the cost of the second check if no sender had group-transaction-events
set to true. In that case, it is irrelevant to return the last transaction
event or null because it will not be used although, it could make more sense to
return the last transaction event.
>
> It looks like the GEODE-7971 tried to use grouping with the existing way
of "invokeTXCallbacks()", where client, wan, cache-listener notification are
done one event from transaction at a time. Instead it could have invoked wan
separately from "firePendingCallbacks"; which has all the TX events in one
group.
> E.g.:
> firePendingCallbacks() {
> invokeTxCallbackForClientCacheLisener(getPendingCallbacks());
> invokeTxCallbackForWan(getPendingCallbacks());
> }
> Then there was no need to tag last transaction event.
> Please correct me if i am missing anything.
>
> And calling "configError" for an TX entry not part of specific sender
seems to be wrong; why would it be a error; think about the case where TX uses
helper/lookup region entry/value to calculate a final result and other site is
only interested in the final result, this is not an error, this is the most
expected behavior.
> E.g.:
> getBonusIncreatementFromBonusRegion();
> updateNewSalaryForEmployee(); // The region or final value other wan site
is interested in.
>
> I think for now we need to change the naming of the methods to be more
meaningful (based on what they are doing):
> "getLastTransactionEvent" to "getLastTransactionEventForGroupedWANSender()"
> Instead of "ServiceConfigurationError" return null or boolean. As the new
change is doing.
Tagging the last transaction event is needed by `SerialGatewaySenderQueue`
and `ParallelGatewaySenderQueue` to make sure that, when
group-transaction-events is enabled, batches do not contain incomplete
transactions. The way to assure that is to check that for every transaction in
the batch (events are tagged with the transaction event), there is an event for
that transaction that is the last event in the transaction (there is an event
tagged as last transaction event).
I do not see why your changes avoid the need for tagging the last
transaction event.
Secondly, having events for a transaction that will not be replicated by all
senders (when some of the senders have group-transaction-events set to true)
could POSSIBLY be a configuration error.
Let me illustrate it with an example:
Let's suppose that we have a system with two senders, sender1 and sender2,
both with group-transaction-events set to true.
Now let's suppose that a transaction (t1) arrives to Geode with two events:
event1 (create a) and event2 (create b).
Let's suppose that event1 is to be replicated by sender1 (but not sender2)
and event 2 is to be replicated by sender2 (but not sender1).
The last event for the transaction will be event2 and it will be tagged as
such.
What we will see happening is that even1 arrives to sender1's queue. When
event1 is added to a batch and the batch is to be sent, it will be checked if
all events for the transaction to which even1 belongs (t1) are in the batch. As
there is only one event and it is not the last event in the the transaction,
the batch creator will try to get the missing events for the transaction but it
will never get them because they will never arrive, the last event in the
transaction will not be on the sender1 queue but it will be on the sender2
queue.
At the same time, event2 will arrive to sender2's queue. When this event is
added to a batch and the batch is to be sent, it will be checked if all
transactions are complete. For t1, as we have the last event for the
transaction (event2), it will be considered that it is so. Nevertheless, as not
all events for the transaction are sent to this sender, the assumption would be
wrong.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
