[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-632576516 > > > The problem with the doc comments was not that they were extensive, but that they got modified when you pasted the diff here and the resulting test was not a valid diff I could apply. > > > > > > I lack permissions to push my commit to your fork. It consists of four edited files. What would you suggest? > > You can send me the diff file attached to my e-mail address. I will apply the diff and then give comments if any here. I just pushed a couple of commits. - One with your comments about the documentation: I accepted all of them, thanks a lot for them. - The second one merging the latest of development. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-631253004 > > The problem with the doc comments was not that they were extensive, but that they got modified when you pasted the diff here and the resulting test was not a valid diff I could apply. > > I lack permissions to push my commit to your fork. It consists of four edited files. What would you suggest? You can send me the diff file attached to my e-mail address. I will apply the diff and then give comments if any here. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-630952955 > > @albertogpz I know the doc comments were extensive. I'll push the changes I'm sure about and dialog with you on points I'm unsure about. One item for you to verify: the references to the `is-parallel` attribute should be to the `parallel` attribute, yes? > > Thanks, Dave. > Yes, you are right: the references to the `is-parallel` attribute should be to the `parallel` attribute The problem with the doc comments was not that they were extensive, but that they got modified when you pasted the diff here and the resulting test was not a valid diff I could apply. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-630952282 > @albertogpz I know the doc comments were extensive. I'll push the changes I'm sure about and dialog with you on points I'm unsure about. One item for you to verify: the references to the `is-parallel` attribute should be to the `parallel` attribute, yes? Thanks, Dave. Yes, you are right: the references to the `is-parallel` attribute should be to the `parallel` attribute 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-630039797 > I've reviewed the doc components and I have some changes to propose. Most are concerned with grammar and format. The only technical one is to replace references to the `is-parallel` property with the `parallel` property. > I'm not sure how best to submit these, as it would be good to have them reviewed before checking them in. The most convenient method for me is the output of a `git diff`, so here goes. If you'd like me to incorporate the material in some other way (e.g. push my local commit to the feature branch) please let me know. > > diff --git a/geode-docs/reference/topics/cache_xml.html.md.erb b/geode-docs/reference/topics/cache_xml.html.md.erb > index 4d9e859550..39e3d66962 100644 > --- a/geode-docs/reference/topics/cache_xml.html.md.erb > +++ b/geode-docs/reference/topics/cache_xml.html.md.erb > @@ -298,8 +298,8 @@ Configures a gateway sender to distribute region events to another <%=vars.produ > group-transaction-events Boolean value to ensure that all the events of a transaction are sent in the same batch, i.e., they are never spread across different batches. - > > Only allowed to be set on gateway senders with `is-parallel` attribute set to false and `dispatcher-threads` attribute equal to 1 or on gatewaysenders with `is-parallel` attribute set to true. > - > > Note that in order to work for a transaction, the regions to which the transaction events belong must be replicated by the same set of senders with this flag enabled. > + > > Only allowed to be set on gateway senders with the `parallel` attribute set to false and `dispatcher-threads` attribute equal to 1, or on gateway senders with the `parallel` attribute set to true. > + > > Note: In order to work for a transaction, the regions to which the transaction events belong must be replicated by the same set of senders with this flag enabled. > false diff --git a/geode-docs/tools_modules/gfsh/command-pages/create.html.md.erb b/geode-docs/tools_modules/gfsh/command-pages/create.html.md.erb index 6140c6f79c..771412a96d 100644 --- a/geode-docs/tools_modules/gfsh/command-pages/create.html.md.erb +++ b/geode-docs/tools_modules/gfsh/command-pages/create.html.md.erb @@ -635,8 +635,8 @@ create gateway-sender --id=value --remote-distributed-system-id=value \-\-group-transaction-events Boolean value to ensure that all the events of a transaction are sent in the same batch, i.e., they are never spread across different batches. - > > Only allowed to be set on gateway senders with `is-parallel` attribute set to false and `dispatcher-threads` attribute equal to 1 or on gatewaysenders with `is-parallel` attribute set to true. > - > > Note that in order to work for a transaction, the regions to which the transaction events belong must be replicated by the same set of senders with this flag enabled. > + > > Only allowed to be set on gateway senders with the `parallel` attribute set to false and `dispatcher-threads` attribute equal to 1, or on gatewaysenders with the `parallel` attribute set to true. > + > > Note: In order to work for a transaction, the regions to which the transaction events belong must be replicated by the same set of senders with this flag enabled. > false diff --git a/geode-docs/topologies_and_comm/multi_site_configuration/setting_up_a_multisite_system.html.md.erb b/geode-docs/topologies_and_comm/multi_site_configuration/setting_up_a_multisite_system.html.md.erb index 64a129009f..be8dfafeb9 100644 --- a/geode-docs/topologies_and_comm/multi_site_configuration/setting_up_a_multisite_system.html.md.erb +++ b/geode-docs/topologies_and_comm/multi_site_configuration/setting_up_a_multisite_system.html.md.erb @@ -171,7 +171,7 @@ When using a multi-site configuration, you choose which data regions to share be > > * Regions using the same parallel gateway sender ID must be colocated. > > > -- If any gateway sender configured for the region has the `group-transaction-events` flag set to true, then the regions involved in transactions must all have the same gateway senders with this flag set to true configured. This requires careful configuration of regions with gateway-senders according to the transactions expected in the system. > +- If any gateway sender configured for the region has the `group-transaction-events` flag set to true, then the regions involved in transactions must all have the same gateway senders configured with this flag set to true. This requires careful configuration of regions with gateway senders according to the transactions expected in the system. > > ``` > **Example**: > Assuming the following scenario: > ``` > > @@ -185,14 +185,21 @@ When using a multi-site configuration, you choose which data
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-626524327 > Methods named `isGroupTransactionEvents` might be better named `shouldGroupTransactionEvents`. Similarly, fields/variables named `isGroupTransactionEvents` might be better named simply `groupTransactionEvents`. I agree. Instead of using shouldGroupTransactionEvents I will use mustGroupTransactionEvents which is a bit shorter. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-621255116 > If I do this: > > * start site 1 with a sender with batch size = 5 > > * do a transaction with 1 customer and 10 orders > > * don't start site 2 immediately > > > I see behavior like below: > > The first call to peek is correct. It returns 11 events. > > Since the batch can't be sent, the batch is recreated. This batch only contains 5 events. I had not thought about this case. I have pushed another commit that should solve the issue. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
albertogpz commented on pull request #4928: URL: https://github.com/apache/geode/pull/4928#issuecomment-620529529 Thanks for your comments. Please, see below: > I ran a test with this scenario: > > * 2 colocated partitioned regions called customer and order attached to a parallel sender > > * 1 replicated region called customer_creation_time attached to a serial sender > I assume that both senders are configured to group transactions. > > The transaction does: > > * put 1 customer into customer region > > * put 10 orders into order region > > * put customer create time into customer_creation_time region > > > Here are some notes from this test: > > GatewaySenderFactoryImpl.configureGatewaySender needs a line like below. Otherwise, the Geode xml case doesn't pass the boolean to the sender. > > ``` > this.attrs.isGroupTransactionEvents = senderCreation.isGroupTransactionEvents() > ``` > Good catch. I have pushed a new commit solving it. > The scenario above is not supported since the transaction is spanning multiple senders. I don't think there is a way to determine that during configuration, but the message that is logged needs additional info: Actually, the reason why the scenario is not supported is not because the transaction is spanning multiple senders. The reason here is that some events go to some senders (the customer and order events go to the parallel sender) and other events go to other senders (the creation time event goes to the serial sender). If each event in the transaction went to the same senders, the scenario would be supported. You could have another scenario in which you configured two parallel senders for customer and order regions, with transactions containing customer and order instances and the transactions would be grouped in batches because all events would go to the same senders (two in this case). > > ``` > [error 2020/04/27 15:45:57.439 PDT tid=0x57] Not all events in transaction go to the same senders that group transactions > ``` > > At least the sender ids should be logged if not which events go to which senders. > > I see `TXLastEventInTransactionUtils.checkAllEventsGoToSameGroupingSenders` is throwing the ServiceConfigurationError. Maybe that ServiceConfigurationError's message can contain this info. Then you can log the error. Maybe even the actual stack. > The new commit contains an extended logging error including the events and, for each event, to which senders it would be sent to. I did not want to add this information initially because I am not sure how meaningful it would be for the one reading it and also because it would add processing time. > Also, I see that the test above succeeds. The batch is sent with the customer and orders. Other than that warning which the customer could easily miss, there is no evidence that all the events in the transaction were not sent in the same batch. Maybe the commit should fail? > It would not be possible at this point to make the commit fail as the processing is done after the commit. If done before, it could slow down transaction operations. Besides, I think it is better to make the operation progress to the other side logging the error which would indicate that there would be a small chance that the events would not be sent in the same batch. This, in turn, could only be a problem when there is a network split. > I'll change my test to remove the serial sender and continue to look at this. 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: us...@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #4928: GEODE-7971: Gw sender deliver TX events atomically to Gw receivers
You are right, the code is very similar (just as it happens with the peek method) but the structures on which they operate are different: On the SerialGatewaySenderQueue it operates on a set of transactionIds while on the ParallelGatewaySenderQueue it operates on a map of . Besides, the difference between the Serial queue and the parallel queue makes it different the access to them. On the parallel you need to pass the bucketId and the partition region. On the serial one, you need to pass the last accessed key so that you do not go through the same elements over and over because those are not removed immediately when peeked from the queue as it happens with the parallel queue. As a consequence I did not find an easy way to reuse code that did not end up making things more complex. [ Full content available at: https://github.com/apache/geode/pull/4928 ] This message was relayed via gitbox.apache.org for notifications@geode.apache.org
