Merge pull request #29814 from vespa-engine/jonmv/less-feed-trrying

Do not retry server errors (500, 502, 504) when feeding

7 files changed, 17 insertions, 26 deletions

diff --git a/client/go/internal/cli/cmd/feed_test.go b/client/go/internal/cli/cmd/feed_test.go
index daf649a0fd1..200a0be7c5d 100644
--- a/client/go/internal/cli/cmd/feed_test.go
+++ b/client/go/internal/cli/cmd/feed_test.go
@@ -83,10 +83,10 @@ func TestFeed(t *testing.T) {
 	assert.Equal(t, want, stdout.String())
 
 	for i := 0; i < 10; i++ {
-		httpClient.NextResponseString(500, `{"message":"it's broken yo"}`)
+		httpClient.NextResponseString(503, `{"message":"it's broken yo"}`)
 	}
 	require.Nil(t, cli.Run("feed", jsonFile1))
-	assert.Equal(t, "feed: got status 500 ({\"message\":\"it's broken yo\"}) for put id:ns:type::doc1: giving up after 10 attempts\n", stderr.String())
+	assert.Equal(t, "feed: got status 503 ({\"message\":\"it's broken yo\"}) for put id:ns:type::doc1: giving up after 10 attempts\n", stderr.String())
 	stderr.Reset()
 	for i := 0; i < 10; i++ {
 		httpClient.NextResponseError(fmt.Errorf("something else is broken"))
diff --git a/client/go/internal/vespa/document/dispatcher.go b/client/go/internal/vespa/document/dispatcher.go
index 786e7c332a4..803b124c825 100644
--- a/client/go/internal/vespa/document/dispatcher.go
+++ b/client/go/internal/vespa/document/dispatcher.go
@@ -116,7 +116,7 @@ func (d *Dispatcher) shouldRetry(op documentOp, result Result) bool {
 	} else if result.HTTPStatus == 429 {
 		d.throttler.Throttled(d.inflightCount.Load())
 		return true
-	} else if result.Err != nil || result.HTTPStatus == 500 || result.HTTPStatus == 502 || result.HTTPStatus == 503 || result.HTTPStatus == 504 {
+	} else if result.Err != nil || result.HTTPStatus == 503 {
 		d.circuitBreaker.Failure()
 		if op.attempts < maxAttempts {
 			return true
diff --git a/client/go/internal/vespa/document/dispatcher_test.go b/client/go/internal/vespa/document/dispatcher_test.go
index 4c52bd759d9..d85432474da 100644
--- a/client/go/internal/vespa/document/dispatcher_test.go
+++ b/client/go/internal/vespa/document/dispatcher_test.go
@@ -38,7 +38,7 @@ func (f *mockFeeder) Send(doc Document) Result {
 	failRequest := (f.failAfterNDocs > 0 && len(f.documents) >= f.failAfterNDocs) ||
 		(f.failCount > 0 && f.sendCount <= f.failCount)
 	if failRequest {
-		result.HTTPStatus = 500
+		result.HTTPStatus = 503
 		result.Status = StatusVespaFailure
 	} else {
 		f.documents = append(f.documents, doc)
diff --git a/vespa-feed-client/src/main/java/ai/vespa/feed/client/impl/HttpRequestStrategy.java b/vespa-feed-client/src/main/java/ai/vespa/feed/client/impl/HttpRequestStrategy.java
index 725462e5d24..4c8a4976a83 100644
--- a/vespa-feed-client/src/main/java/ai/vespa/feed/client/impl/HttpRequestStrategy.java
+++ b/vespa-feed-client/src/main/java/ai/vespa/feed/client/impl/HttpRequestStrategy.java
@@ -166,7 +166,7 @@ class HttpRequestStrategy implements RequestStrategy {
         }
 
         logResponse(FINE, response, request, attempt);
-        if (response.code() == 500 || response.code() == 502 || response.code() == 503 || response.code() == 504) { // Hopefully temporary errors.
+        if (response.code() == 503) { // Hopefully temporary errors.
             breaker.failure(response);
             return retry(request, attempt);
         }
diff --git a/vespa-feed-client/src/test/java/ai/vespa/feed/client/impl/HttpRequestStrategyTest.java b/vespa-feed-client/src/test/java/ai/vespa/feed/client/impl/HttpRequestStrategyTest.java
index 36e81ff4abb..c943e3b139f 100644
--- a/vespa-feed-client/src/test/java/ai/vespa/feed/client/impl/HttpRequestStrategyTest.java
+++ b/vespa-feed-client/src/test/java/ai/vespa/feed/client/impl/HttpRequestStrategyTest.java
@@ -151,7 +151,7 @@ class HttpRequestStrategyTest {
         assertEquals(success, serialised.get());
 
         // Some error responses are retried.
-        HttpResponse serverError = HttpResponse.of(500, null);
+        HttpResponse serverError = HttpResponse.of(503, null);
         cluster.expect((__, vessel) -> vessel.complete(serverError));
         assertEquals(serverError, strategy.enqueue(id1, request).get());
         assertEquals(11, strategy.stats().requests());
@@ -180,7 +180,7 @@ class HttpRequestStrategyTest {
         codes.put(200, 4L);
         codes.put(400, 1L);
         codes.put(429, 2L);
-        codes.put(500, 3L);
+        codes.put(503, 3L);
         assertEquals(codes, stats.responsesByCode());
         assertEquals(3, stats.exceptions());
 
diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
index ca2b0a10c36..19e0c0dc77d 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/DocumentV1ApiHandler.java
@@ -52,6 +52,7 @@ import com.yahoo.documentapi.metrics.DocumentOperationStatus;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.Request;
 import com.yahoo.jdisc.Response;
+import com.yahoo.jdisc.Response.Status;
 import com.yahoo.jdisc.handler.AbstractRequestHandler;
 import com.yahoo.jdisc.handler.BufferedContentChannel;
 import com.yahoo.jdisc.handler.CompletionHandler;
@@ -909,13 +910,6 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
         });
     }
 
-    private static void badGateway(HttpRequest request, Throwable t, ResponseHandler handler) {
-        loggingException(() -> {
-            log.log(FINE, t, () -> "Document access error handling request " + request.getMethod() + " " + request.getUri().getRawPath());
-            JsonResponse.create(request, Exceptions.toMessageString(t), handler).respond(Response.Status.BAD_GATEWAY);
-        });
-    }
-
     private static void timeout(HttpRequest request, String message, ResponseHandler handler) {
         loggingException(() -> {
             log.log(FINE, () -> "Timeout handling request " + request.getMethod() + " " + request.getUri().getRawPath() + ": " + message);
@@ -971,9 +965,6 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
             catch (IllegalArgumentException e) {
                 badRequest(request, e, handler);
             }
-            catch (DispatchException e) {
-                badGateway(request, e, handler);
-            }
             catch (RuntimeException e) {
                 serverError(request, e, handler);
             }
@@ -1097,11 +1088,11 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                     case TIMEOUT -> jsonResponse.commit(Response.Status.GATEWAY_TIMEOUT);
                     case ERROR -> {
                         log.log(FINE, () -> "Exception performing document operation: " + response.getTextMessage());
-                        jsonResponse.commit(Response.Status.BAD_GATEWAY);
+                        jsonResponse.commit(Status.INTERNAL_SERVER_ERROR);
                     }
                     default -> {
                         log.log(WARNING, "Unexpected document API operation outcome '" + response.outcome() + "' " + response.getTextMessage());
-                        jsonResponse.commit(Response.Status.BAD_GATEWAY);
+                        jsonResponse.commit(Status.INTERNAL_SERVER_ERROR);
                     }
                 }
             }
@@ -1402,7 +1393,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                             if (session.get() != null)
                                 response.writeTrace(session.get().getTrace());
 
-                            int status = Response.Status.BAD_GATEWAY;
+                            int status = Status.INTERNAL_SERVER_ERROR;
                             switch (code) {
                                 case TIMEOUT: // Intentional fallthrough.
                                 case ABORTED:
@@ -1539,7 +1530,7 @@ public class DocumentV1ApiHandler extends AbstractRequestHandler {
                 case 404 -> report(DocumentOperationStatus.NOT_FOUND);
                 case 412 -> report(DocumentOperationStatus.CONDITION_FAILED);
                 case 429 -> report(DocumentOperationStatus.TOO_MANY_REQUESTS);
-                case 500,502,503,504,507 -> report(DocumentOperationStatus.SERVER_ERROR);
+                case 500,503,504,507 -> report(DocumentOperationStatus.SERVER_ERROR);
                 default -> throw new IllegalStateException("Unexpected status code '%s'".formatted(response.getStatus()));
             }
             metrics.reportHttpRequest(clientVersion());
diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
index 43b0db1464c..c8fcb4c4635 100644
--- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
+++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/DocumentV1ApiTest.java
@@ -460,7 +460,7 @@ public class DocumentV1ApiTest {
         assertEquals(400, response.getStatus());
 
         // DELETE with namespace and document type is a restricted visit which deletes visited documents.
-        // When visiting fails fatally, a 502 BAD GATEWAY is returned.
+        // When visiting fails fatally, a 500 INTERAL SERVER ERROR is returned.
         access.expect(tokens.subList(0, 1));
         access.expect(parameters -> {
             assertEquals("(false) and (music) and (id.namespace=='space')", parameters.getDocumentSelection());
@@ -485,7 +485,7 @@ public class DocumentV1ApiTest {
                          "message": "boom"
                        }""",
                        response.readAll());
-        assertEquals(502, response.getStatus());
+        assertEquals(500, response.getStatus());
 
         // DELETE at the root is also a deletion visit. These also require a selection.
         access.expect(parameters -> {
@@ -527,7 +527,7 @@ public class DocumentV1ApiTest {
                          "message": "error"
                        }""",
                        response.readAll());
-        assertEquals(502, response.getStatus());
+        assertEquals(500, response.getStatus());
 
         // GET with namespace, document type and number is a restricted visit.
         access.expect(parameters -> {
@@ -931,12 +931,12 @@ public class DocumentV1ApiTest {
                        "  \"pathId\": \"/document/v1/space/music/number/1/two\"," +
                        "  \"message\": \"[FATAL_ERROR @ localhost]: FATAL_ERROR\"" +
                        "}", response1.readAll());
-        assertEquals(502, response1.getStatus());
+        assertEquals(500, response1.getStatus());
         assertSameJson("{" +
                        "  \"pathId\": \"/document/v1/space/music/number/1/two\"," +
                        "  \"message\": \"[FATAL_ERROR @ localhost]: FATAL_ERROR\"" +
                        "}", response2.readAll());
-        assertEquals(502, response2.getStatus());
+        assertEquals(500, response2.getStatus());
 
         // Request response does not arrive before timeout has passed.
         AtomicReference<ResponseHandler> handler = new AtomicReference<>();