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/09/22 22:42:05 UTC
[GitHub] [geode] pivotal-jbarrett opened a new pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
pivotal-jbarrett opened a new pull request #6892:
URL: https://github.com/apache/geode/pull/6892
<!-- 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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r715196997
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -1354,4 +1355,20 @@ protected GatewaySenderEventImpl makeCopy() {
public void setAcked(boolean acked) {
isAcked = acked;
}
+
+ public enum TransactionMetadataDisposition {
+ /**
+ * Transaction metadata should be excluded from the event.
+ */
+ Exclude,
+ /**
+ * Transaction metadata should be included in the event.
+ */
+ Include,
+ /**
+ * Transaction metadata should be included in the event and this is the last event in the
+ * transaction.
+ */
+ IncludeLastEvent,
Review comment:
I would be more include to renaming it `IncludeAsLastEvent` implying both the inclusion of the `TransactionId` and the setting of the `isLastEvent` flag.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r715180491
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -1354,4 +1355,20 @@ protected GatewaySenderEventImpl makeCopy() {
public void setAcked(boolean acked) {
isAcked = acked;
}
+
+ public enum TransactionMetadataDisposition {
+ /**
+ * Transaction metadata should be excluded from the event.
+ */
+ Exclude,
+ /**
+ * Transaction metadata should be included in the event.
+ */
+ Include,
+ /**
+ * Transaction metadata should be included in the event and this is the last event in the
+ * transaction.
+ */
+ IncludeLastEvent,
Review comment:
This name is misleading. We actually don't have a case not to include the last event.
When tx id != null, we should always include it. The original name is "isLastEventInTransaction" which makes sense.
Maybe we should change it to "LastEventInTX"?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett merged pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett merged pull request #6892:
URL: https://github.com/apache/geode/pull/6892
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r715198882
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -322,9 +321,11 @@ public GatewaySenderEventImpl(EnumListenerEvent operation, CacheEvent<?, ?> ce,
}
isConcurrencyConflict = event.isConcurrencyConflict();
- transactionId = event.getTransactionId();
- this.isLastEventInTransaction = isLastEventInTransaction;
-
+ if (transactionMetadataDisposition != Exclude) {
Review comment:
The disposition is now only set to anything other than `Exclude` by when TX grouping is enabled. The `transactionId` is `null` when `transactionMetadataDisposition` is `Exclude`. In `toData` the `transactionId` part is only serialized if it isn't `null`.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r714536933
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -1354,4 +1355,20 @@ protected GatewaySenderEventImpl makeCopy() {
public void setAcked(boolean acked) {
isAcked = acked;
}
+
+ public enum TransactionMetadataDisposition {
Review comment:
You need to update `sanctioned-geode-core-serializables.txt` after adding this enum to `GatewaySenderEventImpl`.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r715184220
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -322,9 +321,11 @@ public GatewaySenderEventImpl(EnumListenerEvent operation, CacheEvent<?, ?> ce,
}
isConcurrencyConflict = event.isConcurrencyConflict();
- transactionId = event.getTransactionId();
- this.isLastEventInTransaction = isLastEventInTransaction;
-
+ if (transactionMetadataDisposition != Exclude) {
Review comment:
BTW, I did not see toData() and fromData() are changed. Why serialization and replication would save data?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r715769761
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -1354,4 +1355,20 @@ protected GatewaySenderEventImpl makeCopy() {
public void setAcked(boolean acked) {
isAcked = acked;
}
+
+ public enum TransactionMetadataDisposition {
+ /**
+ * Transaction metadata should be excluded from the event.
+ */
+ Exclude,
Review comment:
Yes, you are correct. :(
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6892: GEODE-9625: Only serialize transaction metadata when grouping enabled.
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #6892:
URL: https://github.com/apache/geode/pull/6892#discussion_r715750905
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySenderEventProcessor.java
##########
@@ -1225,6 +1228,13 @@ public void closeProcessor() {
}
}
+ protected GatewaySenderEventImpl.TransactionMetadataDisposition getTransactionMetadataDisposition(
+ final boolean isLastEventInTransaction) {
+ return getSender().mustGroupTransactionEvents()
Review comment:
I think this would be easier to understand if you changed it to use if statements.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/wan/GatewaySenderEventImpl.java
##########
@@ -1354,4 +1355,20 @@ protected GatewaySenderEventImpl makeCopy() {
public void setAcked(boolean acked) {
isAcked = acked;
}
+
+ public enum TransactionMetadataDisposition {
+ /**
+ * Transaction metadata should be excluded from the event.
+ */
+ Exclude,
Review comment:
I think the geode style guide says constants should be all upper case. The members of an enum are constants. See https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names
I think you should follow this convention for this new enum
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org