From ed2594169dfb84fe71850adf6ae74e35150ad139 Mon Sep 17 00:00:00 2001 From: Matilda Clerke Date: Thu, 17 Oct 2024 14:12:58 +1100 Subject: [PATCH] 7311: rework partial success handling Signed-off-by: Matilda Clerke --- .../eth/manager/peertask/PeerTask.java | 6 +++--- .../manager/peertask/PeerTaskExecutor.java | 19 +++++++------------ .../PeerTaskExecutorResponseCode.java | 1 - .../peertask/PeerTaskExecutorTest.java | 15 +++++++-------- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java index 40b995503f..1243846ac3 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java @@ -75,9 +75,9 @@ public interface PeerTask { Predicate getPeerRequirementFilter(); /** - * Checks if the supplied result is considered a partial success + * Checks if the supplied result is considered a success * - * @return true if the supplied result is considered a partial success + * @return true if the supplied result is considered a success */ - boolean isPartialSuccess(T result); + boolean isSuccess(T result); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java index 0671791436..984cedccec 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java @@ -40,7 +40,6 @@ public class PeerTaskExecutor { private final PeerTaskRequestSender requestSender; private final LabelledMetric requestTimer; - private final LabelledMetric partialSuccessCounter; private final LabelledMetric timeoutCounter; private final LabelledMetric invalidResponseCounter; private final LabelledMetric internalExceptionCounter; @@ -59,12 +58,6 @@ public class PeerTaskExecutor { "request_time", "Time taken to send a request and receive a response", "taskName"); - partialSuccessCounter = - metricsSystem.createLabelledCounter( - BesuMetricCategory.PEERS, - "partial_success_total", - "Counter of the number of partial success occurred", - "taskName"); timeoutCounter = metricsSystem.createLabelledCounter( BesuMetricCategory.PEERS, @@ -145,16 +138,18 @@ public class PeerTaskExecutor { inflightRequestCountForThisTaskClass.decrementAndGet(); } - if (peerTask.isPartialSuccess(result)) { - partialSuccessCounter.labels(taskClassName).inc(); + if (peerTask.isSuccess(result)) { + peer.recordUsefulResponse(); executorResult = new PeerTaskExecutorResult<>( - Optional.ofNullable(result), PeerTaskExecutorResponseCode.PARTIAL_SUCCESS); + Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); } else { - peer.recordUsefulResponse(); + // At this point, the result is most likely empty. Technically, this is a valid result, so + // we don't penalise the peer, but it's also a useless result, so we return + // INVALID_RESPONSE code executorResult = new PeerTaskExecutorResult<>( - Optional.ofNullable(result), PeerTaskExecutorResponseCode.SUCCESS); + Optional.ofNullable(result), PeerTaskExecutorResponseCode.INVALID_RESPONSE); } } catch (PeerNotConnected e) { diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java index 123c3267c0..327461de15 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResponseCode.java @@ -16,7 +16,6 @@ package org.hyperledger.besu.ethereum.eth.manager.peertask; public enum PeerTaskExecutorResponseCode { SUCCESS, - PARTIAL_SUCCESS, NO_PEER_AVAILABLE, PEER_DISCONNECTED, INTERNAL_SERVER_ERROR, diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java index 853210e685..4226f3a133 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java @@ -73,7 +73,7 @@ public class PeerTaskExecutorTest { Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -102,14 +102,13 @@ public class PeerTaskExecutorTest { Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(true); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(false); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); Assertions.assertNotNull(result); - Assertions.assertTrue(result.result().isPresent()); - Assertions.assertSame(responseObject, result.result().get()); - Assertions.assertEquals(PeerTaskExecutorResponseCode.PARTIAL_SUCCESS, result.responseCode()); + Assertions.assertTrue(result.result().isEmpty()); + Assertions.assertEquals(PeerTaskExecutorResponseCode.INVALID_RESPONSE, result.responseCode()); } @Test @@ -132,7 +131,7 @@ public class PeerTaskExecutorTest { .thenReturn(responseMessageData); Mockito.when(requestMessageData.getCode()).thenReturn(requestMessageDataCode); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -238,7 +237,7 @@ public class PeerTaskExecutorTest { Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, ethPeer)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.executeAgainstPeer(peerTask, ethPeer); @@ -276,7 +275,7 @@ public class PeerTaskExecutorTest { Mockito.when(requestSender.sendRequest(subprotocol, requestMessageData, peer2)) .thenReturn(responseMessageData); Mockito.when(peerTask.parseResponse(responseMessageData)).thenReturn(responseObject); - Mockito.when(peerTask.isPartialSuccess(responseObject)).thenReturn(false); + Mockito.when(peerTask.isSuccess(responseObject)).thenReturn(true); PeerTaskExecutorResult result = peerTaskExecutor.execute(peerTask);