agingade commented on pull request #6659:
URL: https://github.com/apache/geode/pull/6659#issuecomment-872524259
> > @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.
--
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]
