[GitHub] [geode] albertogpz commented on a change in pull request #5509: GEODE-8491: Do not store dropped events in stopped primary gateway se…
albertogpz commented on a change in pull request #5509:
URL: https://github.com/apache/geode/pull/5509#discussion_r495138023
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##
@@ -1118,6 +1115,17 @@ public void distribute(EnumListenerEvent operation,
EntryEventImpl event,
}
}
+ private void recordDroppedEvent(EntryEventImpl event) {
+if (this.eventProcessor != null) {
+ this.eventProcessor.registerEventDroppedInPrimaryQueue(event);
Review comment:
@boglesby and @gesterzhou, friendly reminder :-)
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 a change in pull request #5509: GEODE-8491: Do not store dropped events in stopped primary gateway se…
albertogpz commented on a change in pull request #5509:
URL: https://github.com/apache/geode/pull/5509#discussion_r492080459
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##
@@ -1118,6 +1115,17 @@ public void distribute(EnumListenerEvent operation,
EntryEventImpl event,
}
}
+ private void recordDroppedEvent(EntryEventImpl event) {
+if (this.eventProcessor != null) {
+ this.eventProcessor.registerEventDroppedInPrimaryQueue(event);
Review comment:
@boglesby I reverted the change about letting the batch removal thread
remove the dropped events because I did not find a way to stop the thread when
the cache was closing.
@boglesby and @gesterzhou , could you please approve the PR if you are ok
with the changes? Otherwise, please let me know what else should be changed.
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 a change in pull request #5509: GEODE-8491: Do not store dropped events in stopped primary gateway se…
albertogpz commented on a change in pull request #5509:
URL: https://github.com/apache/geode/pull/5509#discussion_r492080459
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##
@@ -1118,6 +1115,17 @@ public void distribute(EnumListenerEvent operation,
EntryEventImpl event,
}
}
+ private void recordDroppedEvent(EntryEventImpl event) {
+if (this.eventProcessor != null) {
+ this.eventProcessor.registerEventDroppedInPrimaryQueue(event);
Review comment:
@boglesby I reverted the change about letting the batch removal thread
remove the dropped events because I did not find a way to stop the thread when
the cache was closing.
@boglesby and @gesterzhou , could you please approve the PR if you are ok
with the changes? Otherwise, please let me know what else should be changed.
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 a change in pull request #5509: GEODE-8491: Do not store dropped events in stopped primary gateway se…
albertogpz commented on a change in pull request #5509:
URL: https://github.com/apache/geode/pull/5509#discussion_r490270113
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##
@@ -1118,6 +1115,17 @@ public void distribute(EnumListenerEvent operation,
EntryEventImpl event,
}
}
+ private void recordDroppedEvent(EntryEventImpl event) {
+if (this.eventProcessor != null) {
+ this.eventProcessor.registerEventDroppedInPrimaryQueue(event);
Review comment:
How could I know if the cache is closing from inside the stop method of
the ParallelGatewaySender? Is there a way?
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 a change in pull request #5509: GEODE-8491: Do not store dropped events in stopped primary gateway se…
albertogpz commented on a change in pull request #5509:
URL: https://github.com/apache/geode/pull/5509#discussion_r489395559
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##
@@ -1118,6 +1115,17 @@ public void distribute(EnumListenerEvent operation,
EntryEventImpl event,
}
}
+ private void recordDroppedEvent(EntryEventImpl event) {
+if (this.eventProcessor != null) {
+ this.eventProcessor.registerEventDroppedInPrimaryQueue(event);
Review comment:
@boglesby I have made the change you have proposed for the parallel
gateway sender case. You can check it in the new commit. Nevertheless, this
change it requires that the BatchRemoval thread is not shutdown when the
gateway sender is stopped. Is there any problem with not stopping the
BatchRemoval thread when the sender is stopped?
Regarding the SerialGatewaySender, I think I found a way of doing it which
consists of changing the following call in
SerialGatewaySenderEventProcessor.registerEventDroppedInPrimaryQueue():
``
this.processors.get(index).sendBatchDestroyOperationForDroppedEvent(droppedEvent,
index);
``
to this one:
``
this.processors.get(index).sendBatchDestroyOperationForDroppedEvent(droppedEvent,
index);
``
And adding the following method to SerialGatewaySenderQueue:
``
public void addRemovedEvent(EntryEventImpl droppedEvent) {
lock.writeLock().lock();
lastDispatchedKey = droppedEvent.getTailKey();
notifyAll();
lock.writeLock().unlock();
}
``
The problem is that I did not find any test case to verify that it worked
correctly. I ran the tests in SerialWANPropagationsFeatureDUnitTest and it did
not matter if I used the original solution, the new solution or even removed
the sending of batch destroy operation. I looks as if it were not necessary.
That's why I did not dare to add the change. Any idea?
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 a change in pull request #5509: GEODE-8491: Do not store dropped events in stopped primary gateway se…
albertogpz commented on a change in pull request #5509:
URL: https://github.com/apache/geode/pull/5509#discussion_r488052139
##
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/wan/AbstractGatewaySender.java
##
@@ -1118,6 +1115,24 @@ public void distribute(EnumListenerEvent operation,
EntryEventImpl event,
}
}
+ private void recordDroppedEvent(EntryEventImpl event) {
+final boolean isDebugEnabled = logger.isDebugEnabled();
+if (this.eventProcessor != null) {
+ this.eventProcessor.registerEventDroppedInPrimaryQueue(event);
+} else {
+ // Add empty event so that in case the event stays for long in
+ // tmpDroppedEvents it takes as little space as possible.
+ // No need to have all the contents of the event for a dropped one.
+ EntryEventImpl emptyEvent = new EntryEventImpl(event.getKey(), false);
Review comment:
ok. I will remove it.
Anyway, if the gateway sender is created with manualStart=true then the
eventProcessor will not be yet instantiated. In that case, and until the
gateway sender is started we could see dropped events put in tmpDroppedEvents.
It cannot be guaranteed that it will be a small time window.
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
