7311: add peertask foundation code #7628

Matilda-Clerke · 2024-09-17T03:26:45Z

PR description

This PR adds the foundational code for the new peer task system. For details about how this is used, please see the spike PR proving the concept is workable.

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

pinges

Left some comments

pinges · 2024-09-20T01:43:43Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java

+import org.slf4j.LoggerFactory;
+
+/** "Manages" the EthPeers for the PeerTaskExecutor */
+public class PeerManager {


Looking at the code a bit I think that PeerManager could just be an interface with one method: getPeer()
Different implementations could give us the behaviour that we are looking for, like how many retries, retry the same peer/other peers.
This would also allow us to pick peers when they are needed, e.g. to make sure that these peers don't have too many open requests.

An interface to allow easy swapping of different implementations is a fine, but I'm not sure how we'd implement retries in the peer manager... the peer manager is completely separated from the PeerTasks and has no idea what tasks are being executed and whether they succeed or fail.

pinges · 2024-09-20T01:47:17Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+
+  public <T> PeerTaskExecutorResult<T> execute(final PeerTask<T> peerTask) {
+    PeerTaskExecutorResult<T> executorResult;
+    int triesRemaining =


this could be handled by the peer manager

I'm not sure I understand how that would work. The peer manager doesn't have any connection to the tasks being run

pinges · 2024-09-20T02:11:54Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+      }
+    } while (--triesRemaining > 0
+        && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS
+        && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED


If you don't care about which peer is used you would try with another peer ...

This is in the executeAgainstPeer() method, so at this point in the code we do care which peer is used. If the calling code doesn't care which peer a task is run against, it can call the execute() method instead, which contains the logic for peer switching.

pinges · 2024-09-20T02:13:16Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    } while (--triesRemaining > 0
+        && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS
+        && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED
+        && sleepBetweenRetries(WAIT_TIME_BEFORE_RETRY[triesRemaining]));


could be handled by the peer manager

I'm not sure how exactly. The peer manager doesn't (and shouldn't) know anything about whether a peer is requested for a retry or for a first attempt.

Why do we need to sleep between retries?

We don't strictly need to sleep between retries, but if a peer is unable to respond before timing out, it's likely just busy or we're experiencing network issues, so waiting a moment before retrying can allow things a chance to improve.

My understanding was this is a refactor so I think we should match the existing behaviour. Not sure how the retry timing worked before, possibly it is 1 second? https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/task/AbstractRetryingPeerTask.java#L154

If we can later improve the peering performance with different timeouts that would be a good experiment to test out once the refactor is stable.

Instead of Thread.sleep, is there a reason not to use ethScheduler.scheduleFutureTask as before? I think we will lose the ethScheduler metrics if we just use vanilla CompleteableFutures

Instead of Thread.sleep, is there a reason not to use ethScheduler.scheduleFutureTask as before?
Mainly because this particular method shouldn't be concerned with scheduling tasks. Its singular concern is to implement task execution with optional retries.

In addition, methods in the EthSchedule class seem to operate on either Runnables or EthTasks, which really doesn't combine well with the entirely different paradigm at play with PeerTasks and the PeerTaskExecutor. A better solution would be to ensure we have similar metrics set up in the PeerTaskExecutor.

Mainly because this particular method shouldn't be concerned with scheduling tasks.

If we use it, EthScheduler would be the one concerned with scheduling tasks IMO. Anyway isn't this an implementation detail: implementing the retries with a wait time in the PeerTaskExecutor is equivalent to scheduling a future task with a wait time isn't it?

I think we need to maintain the metrics as they are, that's quite a big loss in functionality otherwise. Would much rather we use EthScheduler since a number of maintainers are trying to centralise thread execution here so we can reuse the metrics and testing support in a consistent way across Besu.

Alright, I'll see if I can do a bit of rework to utilise EthScheduler to capture some metrics.

pinges · 2024-09-20T02:14:23Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    }
+  }
+
+  private static boolean isPeerUnused(


should be handled by the peer manager

These methods are here because they are used in the predicate specifically to request peers for the peer task executor. They may not be applicable to all calls to peerManager.getPeer.

They certainly could be moved to PeerManager, but that more tightly couples the peer manager to this one specific use case. Maintaining separation is what gives and maintains the simplicity of this code, as compared with the old system.

pinges · 2024-09-20T02:14:34Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    return !usedEthPeers.contains(ethPeer);
+  }
+
+  private static boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long requiredHeight) {


should be handled by the peer manager

pinges · 2024-09-20T02:14:48Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    return ethPeer.chainState().getEstimatedHeight() >= requiredHeight;
+  }
+
+  private static boolean isPeerProtocolSuitable(final EthPeer ethPeer, final String protocol) {


should be handled by the peer manager

.../src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java

pinges · 2024-09-20T02:18:03Z

.../src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSender.java

+        (boolean streamClosed, MessageData message, EthPeer peer) -> {
+          responseMessageDataFuture.complete(message);
+        });
+    return responseMessageDataFuture.get(timeoutMs, TimeUnit.MILLISECONDS);


I like that!

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

siladu

Ended up making quite a few comments as I went through.

TL;DR: No major objections. The way peers are selected needs some more consideration IMO as it seems a bit spread out across the classes.

siladu · 2024-09-24T03:55:59Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java

+/** "Manages" the EthPeers for the PeerTaskExecutor */
+public interface PeerManager {


nit: I liked @jframe's suggestion of PeerSelector since it makes its role clearer IMO.

"Manager" hides a potential multitude of responsibilities (and may tempt people to expand on them in the future).

This might be a stretch, but I think it also dovetails nicely with the concept of "transaction selection" where we choose transactions to execute from a pool.

Yep, I agree. PeerSelector more accurately describes the classes responsibility.

siladu · 2024-09-24T03:58:31Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java

Thanks for the descriptive comments!

siladu · 2024-09-24T03:59:19Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerManager.java

+public interface PeerManager {
+
+  /**
+   * Gets the highest reputation peer matching the supplies filter


This feels like an implementation detail of the DefaultPeerManager...if the interface requires it to be highest reputation should we rename the method to getHighestReputationPeer?

...or move the comment to DefaultPeerManager

...or merge PeerManager with DefaultPeerManager :)

That's fair. Originally, there was no interface, so the javadoc described the implementation. I'll reword it

siladu · 2024-09-24T04:10:06Z

...eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java

+
+  // use a synchronized map to ensure the map is never modified by multiple threads at once
+  private final Map<PeerId, EthPeer> ethPeersByPeerId =
+      Collections.synchronizedMap(new HashMap<>());


ConcurrentHashMap is a more modern and performant equivalent.

Nice, never use that one before.

siladu · 2024-09-24T04:15:57Z

...eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManager.java

+
+  @Override
+  public EthPeer getPeer(final Predicate<EthPeer> filter) throws NoAvailablePeerException {
+    LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size());


Suggested change

LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size());

LOG.trace("Finding peer from pool of {} peers", ethPeersByPeerId.size());

siladu · 2024-09-24T05:21:27Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+
+/** Manages the execution of PeerTasks, respecting their PeerTaskBehavior */
+public class PeerTaskExecutor {
+  private static final long[] WAIT_TIME_BEFORE_RETRY = {0, 20000, 5000};


What's the thinking behind these retry timings? Warrants a comment I think.

The idea was to have a progressive back-off timer on retries. As discussed elsewhere, it would be better to stay consistent with the existing implemention (appears to be a 1 second delay), and make this sort of change later, so I'll change that shortly.

progressive back-off timer on retries

0 -> 20 seconds -> 5 seconds? Should it {0, 5000, 20000}; instead?

but yeh, think we should avoid changing any functionality as much as possible (or do it in both old and new if it's a known improvement)

Oh, it was using the retry countdown to select it. First retry, the countdown would be 2, so WAIT_TIME_BEFORE_RETRY[triesRemaining] would be 5000

siladu · 2024-09-24T05:21:54Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    } while (--triesRemaining > 0
+        && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.SUCCESS
+        && executorResult.getResponseCode() != PeerTaskExecutorResponseCode.PEER_DISCONNECTED
+        && sleepBetweenRetries(WAIT_TIME_BEFORE_RETRY[triesRemaining]));


Why do we need to sleep between retries?

siladu · 2024-09-24T05:24:41Z

.../test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskRequestSenderTest.java

+                throw new RuntimeException(e);
+              }
+            });
+    Thread.sleep(500);


Why do we need this in the test?

Because of the nature of RequestSender, the actual test has to be performed on another thread than the test thread. The code below this line must be execute after RequestSender has submitted it's callback method to responseStream.then. Since it doesn't make sense to expose a signal just for testing, we can just add a small delay to ensure the code is executed in the required sequence. Obviously, for production code, this is a dirty hack and proper inter-thread signalling should be used.

siladu · 2024-09-24T05:25:18Z

...h/src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorTest.java

Nice coverage!

siladu · 2024-09-24T05:26:45Z

...src/test/java/org/hyperledger/besu/ethereum/eth/manager/peertask/DefaultPeerManagerTest.java

+  }
+
+  @Test
+  public void testGetPeer() throws NoAvailablePeerException {


would be nice to test the exceptional case too

Good call, I'll add another test

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

…ch old implementation Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

jframe · 2024-09-25T06:46:08Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java

+   *
+   * @return the SubProtocol used for this PeerTask
+   */
+  String getSubProtocol();


Can the stronger SubProtocol type be used here? Not sure how much of hassle it is practice though

Iirc I looked at using the SubProtocol type and decided against it. I can't remember why, so I'll take another look.

jframe · 2024-09-25T06:47:43Z

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTask.java

+   *
+   * @return the Collection of behaviors this task is expected to exhibit in the PeetTaskExecutor
+   */
+  Collection<PeerTaskBehavior> getPeerTaskBehaviors();


Think a set would make more sense, don't think it makes sense to have duplicate behaviours

jframe · 2024-09-25T06:50:34Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskBehavior.java

+
+public enum PeerTaskBehavior {
+  RETRY_WITH_SAME_PEER,
+  RETRY_WITH_OTHER_PEERS


Should this also include the non-retry single try behaviour if the intent is to include all the current peer behaviours? Otherwise, like @siladu mentioned, this should be the PeerTaskRetryBehavior

Non-retry behaviour is just the absence of retry behaviours.

I wanted to leave this as PeerTaskBehavior to allow for other behaviors to be added later. They aren't strictly just for describing desired retry behavior, but there aren't currently any other behaviors.

I think if the goal is for the code to be more readable, I would go with more specific/descriptive naming and let whoever may (or may not) add more behaviours in later worry about the appropriate name then.

Alright, that makes sense

jframe · 2024-09-25T06:55:13Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+  public <T> PeerTaskExecutorResult<T> execute(final PeerTask<T> peerTask) {
+    PeerTaskExecutorResult<T> executorResult;
+    int triesRemaining =
+        peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_OTHER_PEERS) ? 3 : 1;


Can a constant be created for the number of retries. Also may want to make the number of retries configurable as I think we use a different number of retries in various places.

Added constants for now. I think I'll hold off on making them configurable until I get a better idea of how/when/why different values are used.

jframe · 2024-09-25T06:57:29Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+                (candidatePeer) ->
+                    isPeerUnused(candidatePeer, usedEthPeers)
+                        && (protocolSpecSupplier.get().isPoS()
+                            || isPeerHeightHighEnough(
+                                candidatePeer, peerTask.getRequiredBlockNumber()))
+                        && isPeerProtocolSuitable(candidatePeer, peerTask.getSubProtocol()));


Agree with @siladu here; the peer selection would more cleanly belong in the PeerSelector

jframe · 2024-09-25T07:05:46Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    MessageData requestMessageData = peerTask.getRequestMessage();
+    PeerTaskExecutorResult<T> executorResult;
+    int triesRemaining =
+        peerTask.getPeerTaskBehaviors().contains(PeerTaskBehavior.RETRY_WITH_SAME_PEER) ? 3 : 1;


Bit unsure about this behaviour. If you specify RETRY_WITH_SAME_PEER and RETRY_WITH_OTHER_PEERS what would happen? It would select a peer and try with that peer up to 3 times and if that fails repeat the process up to 3 times. So it could potentially be trying the request up to 9 times?

Yes, that's correct

jframe · 2024-09-25T07:09:20Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    return !usedEthPeers.contains(ethPeer);
+  }
+
+  private static boolean isPeerHeightHighEnough(final EthPeer ethPeer, final long requiredHeight) {


This method doesn't need to be static

jframe · 2024-09-25T07:09:26Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    return ethPeer.chainState().getEstimatedHeight() >= requiredHeight;
+  }
+
+  private static boolean isPeerProtocolSuitable(final EthPeer ethPeer, final String protocol) {


This method doesn't need to be static

jframe · 2024-09-25T07:09:44Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+    }
+  }
+
+  private static boolean isPeerUnused(


This method doesn't need to be static

jframe · 2024-09-25T07:11:13Z

...src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutorResult.java

+
+import java.util.Optional;
+
+public class PeerTaskExecutorResult<T> {


This would be simpler as a record

It means losing the nice Optional wrapping to guarantee the optional result is always set, but yeah in general it should be a record.

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

siladu · 2024-10-10T22:59:20Z

I think all that's really missing at the moment is the in-flight requests metric mentioned above.

Is an equivalent of this logic needed too?

besu/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java

Lines 268 to 275 in aac7c63

    
           final long actualMinBlockNumber; 
        
           if (minimumBlockNumber > 0 && currentProtocolSpecSupplier.get().isPoS()) { 
        
             // if on PoS do not enforce a min block number, since the estimated chain height of the remote 
        
             // peer is not updated anymore. 
        
             actualMinBlockNumber = 0; 
        
           } else { 
        
             actualMinBlockNumber = minimumBlockNumber; 
        
           }

…xecutor Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Matilda-Clerke · 2024-10-11T00:12:01Z

@siladu As that's a requirement of the individual task, I've handled in the EthPeer filter of each PeerTask. You can see an example in https://github.com/hyperledger/besu/pull/7638/files#diff-65888fb86ed73e694cc6171b8f99c9150241acb7edc94a575a0ea83b4b91849dR111-R115

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

jframe · 2024-10-16T02:27:09Z

metrics/core/src/main/java/org/hyperledger/besu/metrics/prometheus/PrometheusGauge.java

    if (observationsMap.putIfAbsent(List.of(labelValues), valueSupplier) != null) {
      final String labelValuesString = String.join(",", labelValues);
      throw new IllegalArgumentException(
          String.format("A gauge has already been created for label values %s", labelValuesString));
    }
  }

+  @Override
+  public boolean isLabelsObserved(final String... labelValues) {


Couldn't see where this new method is used? Is it in the other PR?

It should be used in PeerTaskExecutor.executeAgainstPeer to add labels for tasks we haven't seen before

pinges · 2024-10-16T05:24:23Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+          MessageData responseMessageData =
+              requestSender.sendRequest(peerTask.getSubProtocol(), requestMessageData, peer);
+
+          result = peerTask.parseResponse(responseMessageData);


It might be a good idea to not do the parsing here. I don't think that would be hard to change. It would make the PeerTaskExecutor almost trivial (no peer handling, no retries). The tasks themselves would have to handle that, but I guess they have to handle the partial success anyways.
Could we have an AbstractTask that does most of that work?
Maybe something we can discuss tomorrow?

The whole point of the executor is handle retries and peer switching for a task. The task classes themselves don't actively do anything, they're more like a collection of methods that do small specific actions or supply values, like generating the request message. Changing this would be a pretty substantial change at this stage, and creating an AbstractPeerTask class is starting to sound extremely similar to the old system we're trying to move away from.

pinges · 2024-10-16T05:30:14Z

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/peertask/PeerTaskExecutor.java

+                  peerTask.getPeerRequirementFilter().test(candidatePeer)
+                      && !usedEthPeers.contains(candidatePeer));
+      if (peer.isEmpty()) {
+        executorResult =


With regard to the 5 second wait for a peer, I think there are cases where you want to do that retry, e.g. if all the matching peers are busy. But I think that could probably be decided by the caller?

pinges · 2024-10-16T05:34:02Z

plugin-api/build.gradle

@@ -71,7 +71,7 @@ Calculated : ${currentHash}
 tasks.register('checkAPIChanges', FileStateChecker) {
  description = "Checks that the API for the Plugin-API project does not change without deliberate thought"
  files = sourceSets.main.allJava.files
-  knownHash = '4jVaj9yW88nHbX0KmTR3dPQRvj9x8Pvh5E9Ry7KRT6w='
+  knownHash = 'VN2JB2HPpEUDQaDvd7QcMkmmgedasVChfA8tnSf1GHU='


what has changed in the plugin API?

Added a method to LabelledGauge to allow us to check whether a label is observed or not

Can we add a default method (returns a dummy false value?) to avoid the breaking change please?

Yep, good idea

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

pinges

LGTM

Matilda-Clerke added 5 commits September 17, 2024 13:22

7311: Add PeerTask system for use in future PRs

4b80016

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Clean up some warnings

a8d5a9f

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

e4be5c0

Merge branch 'main' into 7311-add-peertask-foundation-code

5859444

Merge branch 'main' into 7311-add-peertask-foundation-code

c335cbe

pinges reviewed Sep 20, 2024

View reviewed changes

Matilda-Clerke added 5 commits September 20, 2024 14:49

7311: Reduce timeout in PeerTaskRequestSender to 5s

08c66fd

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Refactor PeerManager to be an interface

049cae2

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

5afba63

Merge branch 'main' into 7311-add-peertask-foundation-code

f2ac53e

Merge branch 'main' into 7311-add-peertask-foundation-code

e901fdf

siladu reviewed Sep 24, 2024

View reviewed changes

Matilda-Clerke added 10 commits September 25, 2024 15:43

Merge branch 'main' into 7311-add-peertask-foundation-code

6e349e1

7311: Rename PeerManager to PeerSelector

ad86ae6

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Reword PeerSelector javadoc to avoid implementation details

38f04ab

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Use ConcurrentHashMap in DefaultPeerSelector

6de3fb3

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Reword trace log in DefaultPeerSelector

da9cd43

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Remove unused imports

ce7d245

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Use a 1 second delay between retries in PeerTaskExecutor to mat…

c9eb22e

…ch old implementation Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest

e2fda73

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest

608fece

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: spotless

2d07800

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

jframe reviewed Sep 25, 2024

View reviewed changes

Matilda-Clerke added 7 commits September 26, 2024 14:40

7311: Fix MetricsAcceptanceTest

ad26297

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Fix MetricsAcceptanceTest

96c8030

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Modify PeerTaskExecutor metric to include response time from peer

b0f2ed0

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Use SubProtocol instead of subprotocol name string in PeerTask

598b519

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: rename timing context to ignored to prevent intellij warnings

bc25b16

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Use constants for number of retries

e31bb70

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Convert PeerTaskExecutorResult to a record

41923d3

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

b1c47ae

7311: Add partialSuccessCounter and inflightRequestGauge in PeerTaskE…

d66dd3a

…xecutor Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

fa22e93

Matilda-Clerke requested review from siladu and jframe October 11, 2024 00:12

Matilda-Clerke added 9 commits October 11, 2024 14:35

7311: Also filter by whether a peer is fully validated

a3f5d4a

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Fix up inflight requests gauge in PeerTaskExecutor

3a68980

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

c422bc5

7311: Update plugin api hash

56c1f9d

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Add javadoc to LabelledGauge.isLabelsObserved

e733452

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Update plugin-api hash

b3a252b

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

4f9cf52

7311: Update changelog

3c96eba

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

7daf30f

jframe reviewed Oct 16, 2024

View reviewed changes

pinges reviewed Oct 16, 2024

View reviewed changes

Matilda-Clerke added 7 commits October 17, 2024 10:17

7311: Use taskName instead of className for labelNames

44fd3a8

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Use snake_case for metric names

ac1c4ed

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Use _total metric name suffix

7503535

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: rework partial success handling

ed25941

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Add default implementation to LabelledGauge.isLabelsObserved

5a79636

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

7311: Fix broken unit test

b075a91

Signed-off-by: Matilda Clerke <matilda.clerke@consensys.net>

Merge branch 'main' into 7311-add-peertask-foundation-code

74995bb

Matilda-Clerke requested review from pinges and jframe October 18, 2024 00:09

jframe approved these changes Oct 18, 2024

View reviewed changes

pinges approved these changes Oct 18, 2024

View reviewed changes

pinges merged commit 2169985 into hyperledger:main Oct 18, 2024
43 checks passed

		/** "Manages" the EthPeers for the PeerTaskExecutor */
		public interface PeerManager {

	LOG.trace("Getting peer from pool of {} peers", ethPeersByPeerId.size());
	LOG.trace("Finding peer from pool of {} peers", ethPeersByPeerId.size());


		import java.util.Optional;

		public class PeerTaskExecutorResult<T> {

7311: add peertask foundation code #7628

7311: add peertask foundation code #7628

Conversation

Matilda-Clerke commented Sep 17, 2024

PR description

pinges left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matilda-Clerke Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Matilda-Clerke Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

siladu Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu left a comment

Choose a reason for hiding this comment

siladu Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siladu commented Oct 10, 2024

Matilda-Clerke commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matilda-Clerke Sep 20, 2024 •

edited

Loading

siladu Sep 25, 2024 •

edited

Loading

Matilda-Clerke Sep 25, 2024 •

edited

Loading

siladu Sep 25, 2024 •

edited

Loading

siladu Sep 24, 2024 •

edited

Loading

siladu Sep 25, 2024 •

edited

Loading