DonalEvans commented on a change in pull request #6625:
URL: https://github.com/apache/geode/pull/6625#discussion_r655754842
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TXCommitMessage.java
##########
@@ -391,9 +416,32 @@ void send(TXLockId lockId) {
setRecipientsSendData(Collections.singleton(indivRecip.next()), processor, rcl);
}
} else {
- // Run in normal mode sending to multiple recipients in
- // one shot
- setRecipientsSendData(recipients, processor, rcl);
+ Set<InternalDistributedMember> tempNotificationOnlyMembers = new
HashSet();
+ Set<InternalDistributedMember> tempTransactionMembers = new
HashSet();
Review comment:
Warnings here can be fixed by using `new HashSet<>();` on both lines.
The angle brackets are necessary to prevent the "raw use of parameterized
class" warning.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TXRegionState.java
##########
@@ -335,16 +345,26 @@ void buildMessage(InternalRegion r, TXCommitMessage msg) {
TXEntryState txes = (TXEntryState) me.getValue();
txes.buildMessage(r, eKey, msg, this.otherMembers);
if (txes.getFilterRoutingInfo() != null) {
+ Set<InternalDistributedMember> tempSet = new HashSet();
Review comment:
This should also be `new HashSet<>();`
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TXRegionState.java
##########
@@ -335,16 +345,26 @@ void buildMessage(InternalRegion r, TXCommitMessage msg) {
TXEntryState txes = (TXEntryState) me.getValue();
txes.buildMessage(r, eKey, msg, this.otherMembers);
if (txes.getFilterRoutingInfo() != null) {
+ Set<InternalDistributedMember> tempSet = new HashSet();
+ if (txes.getAdjunctRecipients() != null) {
+ tempSet.addAll(txes.getAdjunctRecipients());
+ }
+ // exclude members that actually host targeted bucket from
notification only list
+ tempSet.removeAll(redundantMemberSet);
+ if (!tempSet.isEmpty()) {
+ if (msg.getNotificationOnlyMembers().get(regionCommit) == null) {
+ msg.getNotificationOnlyMembers().put(regionCommit, tempSet);
+ } else {
+
msg.getNotificationOnlyMembers().get(regionCommit).addAll(tempSet);
+ }
+ }
Review comment:
If the value returned from `txes.getAdjunctRecipients()` is null, then
we know that `tempSet` will be empty, so we can skip the subsequent
`removeAll()` and `isEmpty()` checks, because we know the outcome of them. With
that in mind, this block can be refactored to:
```
if (txes.getAdjunctRecipients() != null) {
Set<InternalDistributedMember> tempSet = new
HashSet<>(txes.getAdjunctRecipients());
// exclude members that actually host targeted bucket from
notification only list
tempSet.removeAll(redundantMemberSet);
if (!tempSet.isEmpty()) {
if (msg.getNotificationOnlyMembers().get(regionCommit) ==
null) {
msg.getNotificationOnlyMembers().put(regionCommit,
tempSet);
} else {
msg.getNotificationOnlyMembers().get(regionCommit).addAll(tempSet);
}
}
}
newMemberSet.addAll(txes.getFilterRoutingInfo().getMembers());
```
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TXRegionState.java
##########
@@ -319,14 +319,24 @@ void buildMessage(InternalRegion r, TXCommitMessage msg) {
try {
if (!r.getScope().isLocal() && !this.entryMods.isEmpty()) {
- msg.startRegion(r, entryMods.size());
+ TXCommitMessage.RegionCommit regionCommit = msg.startRegion(r,
entryMods.size());
Iterator it = this.entryMods.entrySet().iterator();
- Set<InternalDistributedMember> newMemberSet = new
HashSet<InternalDistributedMember>();
+ Set<InternalDistributedMember> newMemberSet = new HashSet();
+ Set<InternalDistributedMember> redundantMemberSet =
+ new HashSet();
Review comment:
Warnings here can be fixed by using `new HashSet<>();` (with angle
brackets).
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TXRegionState.java
##########
@@ -319,14 +319,24 @@ void buildMessage(InternalRegion r, TXCommitMessage msg) {
try {
if (!r.getScope().isLocal() && !this.entryMods.isEmpty()) {
- msg.startRegion(r, entryMods.size());
+ TXCommitMessage.RegionCommit regionCommit = msg.startRegion(r,
entryMods.size());
Iterator it = this.entryMods.entrySet().iterator();
Review comment:
Can this iterator be moved down to above the while loop that uses it, to
make things a little clearer?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/TXRegionState.java
##########
@@ -319,14 +319,24 @@ void buildMessage(InternalRegion r, TXCommitMessage msg) {
try {
if (!r.getScope().isLocal() && !this.entryMods.isEmpty()) {
- msg.startRegion(r, entryMods.size());
+ TXCommitMessage.RegionCommit regionCommit = msg.startRegion(r,
entryMods.size());
Iterator it = this.entryMods.entrySet().iterator();
- Set<InternalDistributedMember> newMemberSet = new
HashSet<InternalDistributedMember>();
+ Set<InternalDistributedMember> newMemberSet = new HashSet();
+ Set<InternalDistributedMember> redundantMemberSet =
+ new HashSet();
if (r.getScope().isDistributed()) {
DistributedRegion dr = (DistributedRegion) r;
msg.addViewVersion(dr, dr.getDistributionAdvisor().startOperation());
newMemberSet.addAll(dr.getCacheDistributionAdvisor().adviseTX());
+
redundantMemberSet.addAll(dr.getCacheDistributionAdvisor().adviseTX());
Review comment:
Rather than two calls to `dr.getCacheDistributionAdvisor().adviseTX()`,
could this be extracted to a local variable?
--
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:
[email protected]
