The changes to the GatewaySender interface (adding the setMustQueueDroppedEvents() and mustQueueDroppedEvents() methods) constitute a change to a public API, so should be documented in the appropriate section of this RFC. Also, no mention is given to how this new behaviour will be implemented when the Java API is used to stop a gateway sender via the GatewaySender.stop() method, which would lead to inconsistent default behaviour if not addressed.

Good points, Donal. Thanks!

I have updated the RFC describing how to use the new behavior in case the Java API is used. As you can see, there is no magic, but the one using the Java API can get the new behavior by making use of the new method to set the value for the new flag accordingly and doing the required coordination.

I am not very familiar how gateway senders behave, but by the description of the problem here, this may be a naive question: instead of setting up a boolean, can we just check if the gateway sender is stopped or not before we put the event on the queue?

I am the original author who introduce GEODE-4942 and GEODE-5087. I understand current tmpDroppedEvents contains the side-effect which is mentioned in this RFC. However, I don't think to change the APIs to introduce new parameter is a good idea to resolve it. 

To enhance the original design of tmpDroppedEvents to workaround the side-effect, there are some easier ways. The simplest one is to set a timeout. Currently we did not distinguish if the eventProcessor is purposely shutdown or is in normal restarting. We can introduce a stopTime (long) as attribute, which will be set after 

this.eventProcessor.isStopped(). Then before tmpDroppedEvents.add(clonedEvent), we can compare current timestamp with the saved stopTime,
if it's longer than 10 seconds, don't add to tmpDroppedEvents. I introduce the tmpDroppedEvents is only for restarting window, it should not
be long. 10 seocnds is way longer than necessary.

I have a more mature idea to use the timeout to resolve this issue. I noticed AbstractGatewaySender has an attribute startTime, which will be set to current timestamp in start() of SerialGatewaySenderImpl and SerialAEQImpl. But ParallelGatewaySender never used it. We can make use of this attribute. 

1) in start(), set startTime to be current timestamp as SerialGatewaySenderImpl does. 
2) in stop(), set startTime to be (-1)*System.currentTimeMillis()

3) in distribute(): 

if (!isRunning()) {
  if (this.isPrimary()) {
    if (startTime < 0 && System.currentTimeMillis() + startTime < 10000) {
      tmpDroppedEvents.add(clonedEvent);
      if (isDebugEnabled) {
        logger.debug("add to tmpDroppedEvents for evnet {}", clonedEvent);
      }
    }
  }
  if (isDebugEnabled) {
    logger.debug("Returning back without putting into the gateway sender queue:" + event);
  }
  return;
}

We only need to distinguish if the sender is down due to purposely shutdown or in the middle of restarting. Above code is enough to do it. 

We can have more detail discuss if you like. 

Alberto Gomez I also created a draft Pull Request based on the simple solution. It will resolve the problem you mentioned. https://github.com/apache/geode/pull/5381

You can keep refining this fix. 

This fix has minimum impact to current system and APIs. 

Alberto Gomez
Introducing a new stopTime is OK. I just find the startTime is never used and want to use it. 

15 seconds is adjustable. I feel it's safe to increase even to 1 minute. I don't worry that much about the space used for the time being. We can enhance the item's size which are put into the tmpDroppedEvents. We only keep the tmpDroppedEvents to notify secondary queue to delete. So we might not need to clone the whole event's data structure. For example, during clone, we can set some fields to be null. Note: I means to clone a new event dedicated for tmpDroppedEvents and keep the current cloneEvent as is. 

For your question 1, we have not found any bugs for that scenario yet. However, we are not supposed to shutdown one sender for long time. Either we restart it (which should finish within 15 seconds), or shutdown all senders (usually due to shutdown all members). 

In my previous comment, there's a mistake. We cannot clone a simplified event, because the event will be enqueued when sender is restarted. For your test, I will take a look later. 

Alberto Gomez Your test failure will happen when the senders are started at the first time, when the startTime is initialized as 0. vm2 and vm4 could start in different speed, which caused different number of events are rejected at vm2 and vm4. In my reproduce, vm2 reject tailKey==100, 101, but vm4 rejected tailKay=100,101,102. If vm2 happened to be the secondary, then taikKey=102 will not be drained. 

I have fixed it. See the latest code in pull request.  

By the way, in your test code need to adjust a little bit. I have added your test case into the PR. 

I know it might. However in general, I think nothing should be added to the queue if it's not running. If events get added to the secondary that aren't added to the primary, they should be dealt with in the first batch removal message.

Currently our  batch removal message packed a key list. Only the events in the key list at secondary will be removed. We can enhance it to double check if there's any "too-old" events in secondary. Remove them if any. There're 2 alternative to identify an "too-old" event. 1) set a long timeout such as 1 hour.  2) set a boolean firstBatchRemoval to the batch removal message. If it's true, then remove all the events in secondary queue with smaller shadow keys than the smallest one in the batch. 

It could break backward compatibility. And any changes to public API will trigger many other components to be changed. Such as JMX and Restful API. Adding gfsh parameter is the last choice. We would rather to disable the whole tmpDroppedEvents feature to resolve your original issue. 

My fix in this PR have dramatically reduce the chance of undrained event in secondary queue. Only could happen when user stopped one sender (not for restart) then wait for more than 15 seconds to manual restart. Even though, the worst case is the possibility of a few undrained events in secondary queue, and the possibility is way less than totally disabled tmpDroppedEvents. 

As for manually stop one sender, it will always face the risk of missing data. We strongly recommend NOT to do it. You can either shutdown all senders, or restart one sender. 

Alberto Gomez In your solution, if a developer wants to use java API to stop a gateway sender, does he need to set the added boolean flag to a specific value?  It's not very obvious to me what value I need to set it to be, so to me this added boolean flag does add API complexity and confusion.

All these seem to be some internal management of the queue sizes, so I would like to leave users out of it. Requiring users to know the details of the implementation in order to set a flag seems not user-friendly to me.

In your solution, you are using a loop to set the flag on all the members, so it's still possible that the secondary GWS (gateway sender) will get the notice later than the primary, so it could still possibly queue some events before it turns the boolean off, so that event could still not be drained.

As far as I understand, tmpDroppedEvents is used to store events that might have been queued up by the secondary GWS while the primary GWS is down.  Ideally, if user didn't stop the secondary GWS but have the primary GWS stopped, we would have this tmpDroppedEvents store those events indefinitely until the primary starts again,  but if we are concerned the variable will get too big, then setting a timeout seems like a reasonable solution.  The problem of this approach is when user didn't stop all the secondary GWS after the timeout period after they stop the primary GWS, then we will need to ask ourselves: how likely is this to happen?

Alberto Gomez
My proposal is also a draft workaround. It will help the situation in most use cases (if not all). And actually we can apply both solutions. 

I am not against to introduce a new feature to set a status to all senders. However, there are the 4 major problems in your solution:

1. Your solution only changed gfsh command to setMustQueueDroppedEvents on all senders, but not to provide a java API to do it. Your RFC relied on pure java user java application to call the sender.setMustQueueDroppedEvents server by server. This is not the correct way to introduce such a feature. If we need to add a feature to change a status on all senders, we need to have a java API to send message or a function to all members, then let gfsh to call this new java API.
2. Backward compatibility issue: if your cluster has mixed with old version of servers, which does not have this attribute and does not have the setMustQueueDroppedEvents() method either. How you handle it? For example, at least you need to  detect the version and not to call setMustQueueDroppedEvents() for old members.
3. You need to handle that the setMustQueueDroppedEvents(true or false) message or function only executed on some members, but not on other members due to race condition. This error handling should also be considered. Usually we employ a retry. 
4. You also need to handle that setMustQueueDroppedEvents(true or false) is done on different member at different time. There’s a time window that some member has set it to true but other members set it to false. RFC needs to prove that such a time window will not cause problems. 
   
   If considered all above 4 challenges, you will see why my proposal is simpler. 
   
   

There is `ExecuteFunctionCommand` that will allow you to execute user functions on any members. I believe what Gester suggested Option 2 is your PR #5348, with the following changes:

1. do not call your `GatewaySenderManagerFunction` in gfsh commands
2. add a few tests just to make sure you can call `GatewaySenderManagerFunction` using `ExeucteFunctionCommand`, which does the thing you expected.

With you PR #5348, Gester is concerned that calling the function in Stop/StartGatewaySenderCommand might still cause some events being undrained and we don't do enough testing to ensure that's not the case.

Alberto Gomez to register a function for member execution, call FunctionService.registerFunction in an initialization code block.

Solution 2 can be simplified to be:

The tmpDroppedEvents was introduced to save the incoming events temporally before EventProcessor was created when sender is manually started at the first time. When EventProcessor is present (stopping or restarting a sender will keep the EventProcessor until the whole sender is garbage collected), no need to save incoming events into tmpDroppedEvents. 

This proposal has minimum impact to current logic and has no backward compatibility concern. 

The solution finally implemented can be seen in the following JIRA ticket: https://issues.apache.org/jira/browse/GEODE-8491