Event forwarding: success if event read from request body
Before this change, any error in dispatching a forwarded event led to an
error HTTP response code and caused re-sending of the event until the
max number of retries exceeded. When a forwarded event gets streamed to
all listeners, an error may occur after the event was already
successfully streamed to some of them. After an error response code is
sent back, the event forwarding will be retried and this can cause that
an event gets streamed to a client multiple times. When the error
condition persists, a client may get the same event repeatedly for
several hours.
This change proposes that as soon as the forwarded event is successfully
read from the request body, the response code is set to success (2xx).
The event forwarding side shouldn't be responsible for re-sending an
event if the event processing side experiences an error after it
successfully read the event from the request body.
Note that when an event is streamed from the same instance where it was
produced, there are no retries on the event level. Therefore, this
change also fixes the asymmetric behaviour in streaming events when the
event is streamed from the instance where it was produced vs when it is
streamed from the instance where the event was forwarded.
Change-Id: Ie9f264a35d0215e9255f7c5fd98e7e81353a537f
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandler.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandler.java
index 414e795..9f7124e 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandler.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandler.java
@@ -17,7 +17,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.EventDispatcher;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -42,11 +41,13 @@
*
* @param event The event to dispatch
*/
- public void dispatch(Event event) throws PermissionBackendException {
+ public void dispatch(Event event) {
try {
Context.setForwardedEvent(true);
log.atFine().log("dispatching event %s", event.getType());
dispatcher.postEvent(event);
+ } catch (Exception e) {
+ log.atSevere().withCause(e).log("Unable to re-trigger event");
} finally {
Context.unsetForwardedEvent();
}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/jgroups/MessageProcessor.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/jgroups/MessageProcessor.java
index 54096ac..73df3eb 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/jgroups/MessageProcessor.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/jgroups/MessageProcessor.java
@@ -26,7 +26,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.server.events.Event;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gson.Gson;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -108,13 +107,7 @@
} else if (cmd instanceof PostEvent) {
Event event = ((PostEvent) cmd).getEvent();
- try {
- eventHandler.dispatch(event);
- log.atFine().log("Dispatching event %s done", event);
- } catch (PermissionBackendException e) {
- log.atSevere().withCause(e).log("Dispatching event %s failed", event);
- return false;
- }
+ eventHandler.dispatch(event);
} else if (cmd instanceof AddToProjectList) {
String projectName = ((AddToProjectList) cmd).getProjectName();
diff --git a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServlet.java b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServlet.java
index 37f0f20..e2e3302 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServlet.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServlet.java
@@ -24,7 +24,6 @@
import com.google.common.net.MediaType;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.EventGson;
-import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gson.Gson;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -53,10 +52,10 @@
sendError(rsp, SC_UNSUPPORTED_MEDIA_TYPE, "Expecting " + JSON_UTF_8 + " content type");
return;
}
- forwardedEventHandler.dispatch(getEventFromRequest(req));
+ Event event = getEventFromRequest(req);
rsp.setStatus(SC_NO_CONTENT);
- } catch (IOException | PermissionBackendException e) {
- log.atSevere().withCause(e).log("Unable to re-trigger event");
+ forwardedEventHandler.dispatch(event);
+ } catch (IOException e) {
sendError(rsp, SC_BAD_REQUEST, e.getMessage());
}
}
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandlerTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandlerTest.java
index 3e1f8fd..73e3401 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandlerTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/ForwardedEventHandlerTest.java
@@ -15,7 +15,6 @@
package com.ericsson.gerrit.plugins.highavailability.forwarder;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.verify;
@@ -82,9 +81,7 @@
.postEvent(event);
assertThat(Context.isForwardedEvent()).isFalse();
- PermissionBackendException thrown =
- assertThrows(PermissionBackendException.class, () -> handler.dispatch(event));
- assertThat(thrown).hasMessageThat().isEqualTo("someMessage");
+ handler.dispatch(event);
assertThat(Context.isForwardedEvent()).isFalse();
verify(dispatcherMock).postEvent(event);
diff --git a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServletTest.java b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServletTest.java
index 3c6b931..7c23e29 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServletTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/highavailability/forwarder/rest/EventRestApiServletTest.java
@@ -26,6 +26,7 @@
import com.ericsson.gerrit.plugins.highavailability.forwarder.ForwardedEventHandler;
import com.google.common.net.MediaType;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.events.EventDispatcher;
import com.google.gerrit.server.events.EventGsonProvider;
import com.google.gerrit.server.events.EventTypes;
import com.google.gerrit.server.events.RefEvent;
@@ -41,6 +42,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
+import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;
@RunWith(MockitoJUnitRunner.class)
@@ -85,11 +87,15 @@
+ "\"refs/changes/76/669676/2\",\"nodesCount\":1,\"type\":"
+ "\"ref-replication-done\",\"eventCreatedOn\":1451415011}";
when(requestMock.getReader()).thenReturn(new BufferedReader(new StringReader(event)));
+
+ EventDispatcher dispatcher = Mockito.mock(EventDispatcher.class);
doThrow(new PermissionBackendException(ERR_MSG))
- .when(forwardedEventHandlerMock)
- .dispatch(any(RefReplicationDoneEvent.class));
+ .when(dispatcher)
+ .postEvent(any(RefReplicationDoneEvent.class));
+ ForwardedEventHandler forwardedEventHandler = new ForwardedEventHandler(dispatcher);
+ eventRestApiServlet = new EventRestApiServlet(forwardedEventHandler, gson);
eventRestApiServlet.doPost(requestMock, responseMock);
- verify(responseMock).sendError(SC_BAD_REQUEST, ERR_MSG);
+ verify(responseMock).setStatus(SC_NO_CONTENT);
}
@Test
