kirklund commented on a change in pull request #6473:
URL: https://github.com/apache/geode/pull/6473#discussion_r650166221
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderEventProcessor.java
##########
@@ -136,14 +136,14 @@ protected boolean enqueueEvent(GatewayQueueEvent
gatewayQueueEvent,
boolean queuedEvent = false;
try {
if (getSender().beforeEnqueue(gatewayQueueEvent)) {
+ if (condition != null &&
+ !((ParallelGatewaySenderQueue) this.queue).hasEventsMatching(
+ (GatewaySenderEventImpl) gatewayQueueEvent,
+ condition)) {
+ return false;
+ }
Review comment:
It might be best to some casts before the conditional just in order to
improve readability:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue)
this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching((GatewaySenderEventImpl)
gatewayQueueEvent, condition)) {
return false;
}
```
We also need to consider updating interfaces like `RegionQueue` to avoid
having to cast gatewayQueueEvent to GatewaySenderEventImpl.
GatewaySenderEventImpl implements `InternalGatewayQueueEvent`. The interfaces
start to lose value very fast if we let them become out-of-date. We should
strive to use interfaces everywhere instead of concrete implementations.
Add:
```
boolean hasEventsMatching(GatewaySenderEventImpl event,
Predicate<InternalGatewayQueueEvent> condition);
```
to `RegionQueue` (if that makes sense... I'm not an expert in Wan code).
Next change that method signature to:
```
boolean hasEventsMatching(InternalGatewayQueueEvent event,
Predicate<InternalGatewayQueueEvent> condition);
```
You can start out by casting the arg `event` within the implementation of
that method, but it's good to push that change through all of the code that
`event` travels to. If any of those methods need implementation specifics, then
update the interface `InternalGatewayQueueEvent`.
Then that conditional block becomes:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue)
this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching(gatewayQueueEvent, condition)) {
return false;
}
```
You could even add `hasNoEventsMatching(...)` to ParallelGatewaySenderQueue.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderEventProcessor.java
##########
@@ -136,14 +136,14 @@ protected boolean enqueueEvent(GatewayQueueEvent
gatewayQueueEvent,
boolean queuedEvent = false;
try {
if (getSender().beforeEnqueue(gatewayQueueEvent)) {
+ if (condition != null &&
+ !((ParallelGatewaySenderQueue) this.queue).hasEventsMatching(
+ (GatewaySenderEventImpl) gatewayQueueEvent,
+ condition)) {
+ return false;
+ }
Review comment:
It might be best to do some casts before conditionals just in order to
improve readability. Example:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue)
this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching((GatewaySenderEventImpl)
gatewayQueueEvent, condition)) {
return false;
}
```
We also need to consider updating interfaces like `RegionQueue` to avoid
having to cast gatewayQueueEvent to GatewaySenderEventImpl.
GatewaySenderEventImpl implements `InternalGatewayQueueEvent`. The interfaces
start to lose value very fast if we let them become out-of-date. We should
strive to use interfaces everywhere instead of concrete implementations.
Add:
```
boolean hasEventsMatching(GatewaySenderEventImpl event,
Predicate<InternalGatewayQueueEvent> condition);
```
to `RegionQueue` (if that makes sense... I'm not an expert in Wan code).
Next change that method signature to:
```
boolean hasEventsMatching(InternalGatewayQueueEvent event,
Predicate<InternalGatewayQueueEvent> condition);
```
You can start out by casting the arg `event` within the implementation of
that method, but it's good to push that change through all of the code that
`event` travels to. If any of those methods need implementation specifics, then
update the interface `InternalGatewayQueueEvent`.
Then that conditional block becomes:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue)
this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching(gatewayQueueEvent, condition)) {
return false;
}
```
You could even add `hasNoEventsMatching(...)` to ParallelGatewaySenderQueue.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/parallel/ParallelGatewaySenderEventProcessor.java
##########
@@ -136,14 +136,14 @@ protected boolean enqueueEvent(GatewayQueueEvent
gatewayQueueEvent,
boolean queuedEvent = false;
try {
if (getSender().beforeEnqueue(gatewayQueueEvent)) {
+ if (condition != null &&
+ !((ParallelGatewaySenderQueue) this.queue).hasEventsMatching(
+ (GatewaySenderEventImpl) gatewayQueueEvent,
+ condition)) {
+ return false;
+ }
Review comment:
It might be best to do some casts before conditionals just in order to
improve readability. Example:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue)
this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching((GatewaySenderEventImpl)
gatewayQueueEvent, condition)) {
return false;
}
```
We also need to consider updating interfaces like `RegionQueue` to avoid
having to cast gatewayQueueEvent to GatewaySenderEventImpl.
GatewaySenderEventImpl implements `InternalGatewayQueueEvent`. The interfaces
start to lose value very fast if we let them become out-of-date. We should
strive to use interfaces everywhere instead of concrete implementations.
Add:
```
boolean hasEventsMatching(GatewaySenderEventImpl event,
Predicate<InternalGatewayQueueEvent> condition);
```
to `RegionQueue` (if that makes sense... I'm not an expert in Wan code).
Next change that method signature to:
```
boolean hasEventsMatching(InternalGatewayQueueEvent event,
Predicate<InternalGatewayQueueEvent> condition);
```
You can start out by casting the arg `event` within the implementation of
that method, but it's good to push that change through all of the code that
`event` travels to. If any of those methods need implementation specifics, then
update the interface `InternalGatewayQueueEvent`.
Then that conditional block becomes:
```
ParallelGatewaySenderQueue parallelQueue = (ParallelGatewaySenderQueue)
this.queue;
if (condition != null &&
!(parallelQueue.hasEventsMatching(gatewayQueueEvent, condition)) {
return false;
}
```
The main point here is to always be updating the interfaces and don't let
the implementation diverge so much that we need to use implementations instead
of the interfaces.
--
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]
