Bill commented on a change in pull request #6310:
URL: https://github.com/apache/geode/pull/6310#discussion_r612778870
##########
File path:
geode-serialization/src/main/java/org/apache/geode/internal/serialization/DataSerializableFixedID.java
##########
@@ -56,6 +56,11 @@
// NOTE, codes < -65536 will take 4 bytes to serialize
// NOTE, codes < -128 will take 2 bytes to serialize
+ short CLEAR_PARTITIONED_REGION_REPLY_MESSAGE = -166;
+ short CLEAR_PARTITIONED_REGION_MESSAGE = -165;
+
+ short PR_CLEAR_REPLY_MESSAGE = -164;
+ short PR_CLEAR_MESSAGE = -163;
Review comment:
These two constants `PR_CLEAR_REPLY_MESSAGE` and `PR_CLEAR_MESSAGE`
aren't referenced anywhere. I think they are superfluous
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClearMessage.java
##########
@@ -139,119 +136,118 @@ public int getDSFID() {
}
@Override
- public void fromData(DataInput in,
- DeserializationContext context) throws IOException,
ClassNotFoundException {
+ public void fromData(DataInput in, DeserializationContext context)
+ throws IOException, ClassNotFoundException {
super.fromData(in, context);
- this.cbArg = DataSerializer.readObject(in);
- op = PartitionedRegionClearMessage.OperationType.values()[in.readByte()];
- eventID = DataSerializer.readObject(in);
+ callbackArgument = DataSerializer.readObject(in);
+ operationType =
PartitionedRegionClearMessage.OperationType.values()[in.readByte()];
+ eventId = DataSerializer.readObject(in);
}
@Override
- public void toData(DataOutput out,
- SerializationContext context) throws IOException {
+ public void toData(DataOutput out, SerializationContext context) throws
IOException {
super.toData(out, context);
- DataSerializer.writeObject(this.cbArg, out);
- out.writeByte(op.ordinal());
- DataSerializer.writeObject(eventID, out);
+ DataSerializer.writeObject(callbackArgument, out);
+ out.writeByte(operationType.ordinal());
+ DataSerializer.writeObject(eventId, out);
+ }
+
+ @Override
+ protected void sendReply(InternalDistributedMember recipient, int
processorId,
+ DistributionManager distributionManager, ReplyException replyException,
+ PartitionedRegion partitionedRegion, long startTime) {
+ if (partitionedRegion != null && startTime > 0) {
+ partitionedRegion.getPrStats().endPartitionMessagesProcessing(startTime);
+ }
+ PartitionedRegionClearMessage.PartitionedRegionClearReplyMessage
+ .send(recipient, processorId, getReplySender(distributionManager),
operationType,
+ bucketsCleared, replyException);
}
/**
* The response on which to wait for all the replies. This response ignores
any exceptions
* received from the "far side"
*/
public static class PartitionedRegionClearResponse extends ReplyProcessor21 {
+
CopyOnWriteHashSet<Integer> bucketsCleared = new CopyOnWriteHashSet<>();
public PartitionedRegionClearResponse(InternalDistributedSystem system,
- Set<InternalDistributedMember> initMembers) {
- super(system, initMembers);
+ Set<InternalDistributedMember> recipients) {
+ super(system, recipients);
}
@Override
- public void process(DistributionMessage msg) {
- if (msg instanceof PartitionedRegionClearReplyMessage) {
- Set<Integer> buckets = ((PartitionedRegionClearReplyMessage)
msg).bucketsCleared;
+ public void process(DistributionMessage message) {
+ if (message instanceof PartitionedRegionClearReplyMessage) {
+ Set<Integer> buckets = ((PartitionedRegionClearReplyMessage)
message).bucketsCleared;
if (buckets != null) {
bucketsCleared.addAll(buckets);
}
}
- super.process(msg, true);
- }
- }
-
- @Override
- protected void sendReply(InternalDistributedMember member, int processorId,
- DistributionManager distributionManager, ReplyException ex,
- PartitionedRegion partitionedRegion, long startTime) {
- if (partitionedRegion != null) {
- if (startTime > 0) {
-
partitionedRegion.getPrStats().endPartitionMessagesProcessing(startTime);
- }
+ process(message, true);
}
- PartitionedRegionClearMessage.PartitionedRegionClearReplyMessage
- .send(member, processorId, getReplySender(distributionManager), op,
bucketsCleared,
- ex);
}
public static class PartitionedRegionClearReplyMessage extends ReplyMessage {
private Set<Integer> bucketsCleared;
- private OperationType op;
+ private OperationType operationType;
@Override
public boolean getInlineProcess() {
return true;
}
+ public static void send(InternalDistributedMember recipient, int
processorId,
+ ReplySender replySender, OperationType operationType, Set<Integer>
bucketsCleared,
+ ReplyException replyException) {
+ Objects.requireNonNull(recipient, "partitionedRegionClearReplyMessage
NULL reply message");
+
+ PartitionedRegionClearMessage.PartitionedRegionClearReplyMessage
replyMessage =
+ new
PartitionedRegionClearMessage.PartitionedRegionClearReplyMessage(processorId,
+ operationType, bucketsCleared, replyException);
+
+ replyMessage.setRecipient(recipient);
+ replySender.putOutgoing(replyMessage);
+ }
+
/**
* Empty constructor to conform to DataSerializable interface
*/
- public PartitionedRegionClearReplyMessage() {}
+ public PartitionedRegionClearReplyMessage() {
+ // Empty constructor to conform to DataSerializable interface
+ }
- private PartitionedRegionClearReplyMessage(int processorId, OperationType
op,
- Set<Integer> bucketsCleared, ReplyException ex) {
- super();
+ private PartitionedRegionClearReplyMessage(int processorId, OperationType
operationType,
+ Set<Integer> bucketsCleared, ReplyException replyException) {
this.bucketsCleared = bucketsCleared;
- this.op = op;
+ this.operationType = operationType;
setProcessorId(processorId);
- setException(ex);
- }
-
- /** Send an ack */
- public static void send(InternalDistributedMember recipient, int
processorId, ReplySender dm,
- OperationType op, Set<Integer> bucketsCleared, ReplyException ex) {
-
- Assert.assertTrue(recipient != null, "partitionedRegionClearReplyMessage
NULL reply message");
-
- PartitionedRegionClearMessage.PartitionedRegionClearReplyMessage m =
- new
PartitionedRegionClearMessage.PartitionedRegionClearReplyMessage(processorId,
op,
- bucketsCleared, ex);
-
- m.setRecipient(recipient);
- dm.putOutgoing(m);
+ setException(replyException);
}
/**
* Processes this message. This method is invoked by the receiver of the
message.
*
- * @param dm the distribution manager that is processing the message.
+ * @param distributionManager the distribution manager that is processing
the message.
*/
@Override
- public void process(final DistributionManager dm, final ReplyProcessor21
rp) {
- final long startTime = getTimestamp();
+ public void process(DistributionManager distributionManager,
+ ReplyProcessor21 replyProcessor21) {
Review comment:
The description of this commit says it "(removes) unnecessary uses of
final…".
There is no such thing as an unnecessary use of `final`. If it is possible
to declare a variable `final` then it is always valuable to do so since it
reduces maintenance effort.
Please put the `final`s back.
##########
File path:
geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt
##########
@@ -79,6 +79,7 @@
org/apache/geode/cache/NoSubscriptionServersAvailableException,true,848408601915
org/apache/geode/cache/Operation,true,-7521751729852504238,ordinal:byte
org/apache/geode/cache/OperationAbortedException,true,-8293166225026556949
org/apache/geode/cache/PartitionedRegionDistributionException,true,-3004093739855972548
+org/apache/geode/cache/PartitionedRegionPartialClearException,false
Review comment:
`PartitionedRegionPartialClearException` was added to
`sanctioned-geode-core-serializables.txt` which implies it is serialized. If it
wasn't serialized then it should go into `excludedClasses.txt` I believe.
Ok so it's serialized. But why doesn't it get its own `serialVersionUID`? I
see that `PartitionedRegionStorageException` has one. Both
`PartitionedRegionStorageException` and
`PartitionedRegionPartialClearException` are derived from
`CacheRuntimeException` and neither defines any extra fields.
I think `PartitionedRegionPartialClearException` needs to define a
`serialVersionUID`.
##########
File path:
geode-core/src/main/resources/org/apache/geode/internal/sanctioned-geode-core-serializables.txt
##########
@@ -302,6 +303,7 @@
org/apache/geode/internal/cache/PRContainsValueFunction,false
org/apache/geode/internal/cache/PRHARedundancyProvider$ArrayListWithClearState,true,1,wasCleared:boolean
org/apache/geode/internal/cache/PartitionedRegion$PRIdMap,true,3667357372967498179,cleared:boolean
org/apache/geode/internal/cache/PartitionedRegion$SizeEntry,false,isPrimary:boolean,size:int
+org/apache/geode/internal/cache/PartitionedRegionClearMessage$OperationType,false
Review comment:
ok ✓ (enums don't need `serialVersionUID`.)
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClearMessage.java
##########
@@ -41,96 +42,92 @@
import org.apache.geode.internal.serialization.SerializationContext;
import org.apache.geode.logging.internal.log4j.api.LogService;
-/**
- * this message is for operations no the partition region level, could be sent
by any originating
- * member to the other members hosting this partition region
- */
public class PartitionedRegionClearMessage extends PartitionMessage {
+ private static final Logger logger = LogService.getLogger();
public enum OperationType {
OP_LOCK_FOR_PR_CLEAR, OP_UNLOCK_FOR_PR_CLEAR, OP_PR_CLEAR,
}
- private Object cbArg;
-
- private OperationType op;
-
- private EventID eventID;
-
+ private Object callbackArgument;
+ private OperationType operationType;
+ private EventID eventId;
private PartitionedRegion partitionedRegion;
-
private Set<Integer> bucketsCleared;
- @Override
- public EventID getEventID() {
- return eventID;
+ public PartitionedRegionClearMessage() {
+ // nothing
}
- public PartitionedRegionClearMessage() {}
+ PartitionedRegionClearMessage(Set<InternalDistributedMember> recipients,
+ PartitionedRegion partitionedRegion, ReplyProcessor21 replyProcessor21,
+ PartitionedRegionClearMessage.OperationType operationType,
RegionEventImpl regionEvent) {
Review comment:
why was the `final` removed from the `RegionEventImpl` parameter?
If it's consistency you're after, then I recommend making all the parameters
to this method `final`.
--
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]
