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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

…ch old implementation Signed-off-by: Matilda Clerke <[email protected]>

Signed-off-by: Matilda Clerke <[email protected]>

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 <[email protected]>

siladu · 2024-10-04T00:44:16Z

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

+ public EthPeer getPeer(final Predicate<EthPeer> filter) {
+ return streamBestPeers().filter(filter).findFirst().orElseThrow(NoAvailablePeersException::new);
+ }


Rather than add a duplicate implementation, could we just use the existing bestPeerMatchingCriteria or refactor to suit both needs?

My preference would be Optional<EthPeer> instead of an exception for normal control flow, and reusing the existing impl would mean we less risk/testing for the existing clients of that method.

Ok, I'll update

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

siladu · 2024-10-04T00:59:59Z

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

+
+ public static final int RETRIES_WITH_SAME_PEER = 3;
+ public static final int RETRIES_WITH_OTHER_PEER = 3;
+ public static final int NO_RETRIES = 1;


nit: I think the benefit is succinctness/readability/encapsulation. If I want to understand what PeerTaskRetryBehavior.RETRIES_WITH_SAME_PEER actually does then I need to understand this extra mapping within PeerTaskExecutor, whereas if the enum contains a numberOfRetries field, this should be more obvious IMO.

Signed-off-by: Matilda Clerke <[email protected]>

…isting peer selection behavior in EthPeers Signed-off-by: Matilda Clerke <[email protected]>

siladu

After discussion and further review, it sounds like this code is bypassing some existing functionality in EthPeers. That might be fine if done intentionally, but I'd like to understand why it is fine to skip it.

For example, I think it is missing an equivalent to EthPeer.hasAvailableRequestCapacity.

Also EthPeer.executePeerRequest has some logic around actualMinBlockNumber and maintains a synchronized Collection<PendingPeerRequest> pendingRequests which also has associated metrics - is there an equivalent to these in the new system? Maybe they are superfluous?

Without a good understanding of the old system, in particular EthPeers, it is hard to understand from the new system what is being bypassed, and whether it is intentional or something that was missed. I will have to defer to you @Matilda-Clerke to point these kind of changes out, or maybe for @pinges to spot them.

Signed-off-by: Matilda Clerke <[email protected]>

Matilda-Clerke · 2024-10-08T05:17:56Z

I think it is missing an equivalent to EthPeer.hasAvailableRequestCapacity
I've added this check in getPeer, but it does mean we can't just reuse the bestPeerMatchingCriteria method as you suggested.

EthPeer.executePeerRequest
There definitely can be an equivalent in the new system. Alternative question though: Is that metric actually useful to anyone? The only metric I can find is just monitoring the size of the List, i.e. monitoring how many PeerRequests are "in flight".
The logic around minimum chain height, I did have implemented in PeerTaskExecutor when selecting a peer, but when we moved to the PeerTask providing a Predicate to filter EthPeers, it wasn't included. I'll have to find a way to get the required information into the places utilising GetReceiptsFromPeerTask so it can be passed in when the task is created.

We definitely need as many eyes on these changes as we can get. I think all that's really missing at the moment is the in-flight requests metric mentioned above.

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

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

jframe · 2024-10-08T05:24:59Z

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

+ private boolean sleepBetweenRetries() {
+ try {
+ // sleep for 1 second to match implemented wait between retries in AbstractRetryingPeerTask
+ Thread.sleep(1000);


Could we just use ethContext.getScheduler().scheduleFutureTask so we don't have to block entire thread for a retry

It's possible. In practice, this is only called on a worker thread anyway

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

jframe · 2024-10-08T05:28:50Z

...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 =


In the previous code, we would have an additional wait of 5 seconds if there were no peers. Is this still needed? @pinges what do you think?

Yeah I noticed that. Seems very weird to me that we'd just sit and wait for a peer, but only for 5 seconds. Imo, we should just let the task/pipeline fail so it can be retried later.

jframe · 2024-10-08T05:31:56Z

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

+ && !usedEthPeers.contains(candidatePeer));
+ if (peer.isEmpty()) {
+ executorResult =
+ new PeerTaskExecutorResult<>(


This doesn't handle disconnecting the failed peers like the AbstractRetryingSwitchingPeerTask.refreshPeers does in this case

No, disconnecting failed peers shouldn't be done here (or in AbstractRetryingSwitchingPeerTask). It's just not one of the responsibilities of a class which is managing the sending of a request message and parsing of the response.

jframe · 2024-10-08T05:34:25Z

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

+ Optional<EthPeer> peer =
+ peerSelector.getPeer(
+ (candidatePeer) ->
+ peerTask.getPeerRequirementFilter().test(candidatePeer)


How will the isSuitablePeer check intended to be handled? Will this happen through the getPeerRequirementFilter

Yes, all of a tasks requirements should be included in the getPeerRequirementFilter Predicate, and any additional requirements should be added here by the PeerTaskExecutor

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

Signed-off-by: Matilda Clerke <[email protected]>

…class name Signed-off-by: Matilda Clerke <[email protected]>

…or enums Signed-off-by: Matilda Clerke <[email protected]>

Signed-off-by: Matilda Clerke <[email protected]>

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 <[email protected]>

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

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 <[email protected]>

7311: Clean up some warnings

a8d5a9f

Signed-off-by: Matilda Clerke <[email protected]>

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 <[email protected]>

7311: Refactor PeerManager to be an interface

049cae2

Signed-off-by: Matilda Clerke <[email protected]>

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 <[email protected]>

7311: Reword PeerSelector javadoc to avoid implementation details

38f04ab

Signed-off-by: Matilda Clerke <[email protected]>

7311: Use ConcurrentHashMap in DefaultPeerSelector

6de3fb3

Signed-off-by: Matilda Clerke <[email protected]>

7311: Reword trace log in DefaultPeerSelector

da9cd43

Signed-off-by: Matilda Clerke <[email protected]>

7311: Remove unused imports

ce7d245

Signed-off-by: Matilda Clerke <[email protected]>

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

c9eb22e

…ch old implementation Signed-off-by: Matilda Clerke <[email protected]>

7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest

e2fda73

Signed-off-by: Matilda Clerke <[email protected]>

7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest

608fece

Signed-off-by: Matilda Clerke <[email protected]>

7311: spotless

2d07800

Signed-off-by: Matilda Clerke <[email protected]>

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 <[email protected]>

7311: Fix MetricsAcceptanceTest

96c8030

Signed-off-by: Matilda Clerke <[email protected]>

7311: Modify PeerTaskExecutor metric to include response time from peer

b0f2ed0

Signed-off-by: Matilda Clerke <[email protected]>

7311: Use SubProtocol instead of subprotocol name string in PeerTask

598b519

Signed-off-by: Matilda Clerke <[email protected]>

7311: rename timing context to ignored to prevent intellij warnings

bc25b16

Signed-off-by: Matilda Clerke <[email protected]>

7311: Use constants for number of retries

e31bb70

Signed-off-by: Matilda Clerke <[email protected]>

7311: Convert PeerTaskExecutorResult to a record

41923d3

Signed-off-by: Matilda Clerke <[email protected]>

Matilda-Clerke added 2 commits October 7, 2024 09:12

7311: Rework PeerTaskExecutor retry system to be 0-based

0896e31

Signed-off-by: Matilda Clerke <[email protected]>

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

5f924c4

Matilda-Clerke mentioned this pull request Oct 6, 2024

7311: Build new peer task system with proof of concept example task implementation #7602

Closed

Matilda-Clerke added 2 commits October 8, 2024 08:54

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

2865625

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

82cedb0

Matilda-Clerke requested a review from siladu October 7, 2024 22:55

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

bdd96ba

siladu reviewed Oct 8, 2024

View reviewed changes

Matilda-Clerke added 2 commits October 8, 2024 12:37

7311: Remove unused async methods in PeerTaskExecutor

c047f42

Signed-off-by: Matilda Clerke <[email protected]>

7311: Return Optional<EthPeer> in PeerSelector.getPeer and utilise ex…

5aa6b0b

…isting peer selection behavior in EthPeers Signed-off-by: Matilda Clerke <[email protected]>

siladu reviewed Oct 8, 2024

View reviewed changes

This was referenced Oct 8, 2024

7311: add GetReceiptsFromPeerTask #7638

Open

Fix unhandled exception #7743

Merged

7311: Redo getPeer again to include hasAvailableRequestCapacity check

8becdb3

Signed-off-by: Matilda Clerke <[email protected]>

jframe reviewed Oct 8, 2024

View reviewed changes

Matilda-Clerke added 8 commits October 9, 2024 10:29

7311: Rework getPeer again to use LEAST_TO_MOST_BUSY comparator

8186a77

Signed-off-by: Matilda Clerke <[email protected]>

7311: Import PeerNotConnected class instead of using fully qualified …

37b0ec2

…class name Signed-off-by: Matilda Clerke <[email protected]>

7311: Change to specifying retry counts in PeerTask instead of behavi…

545fd5c

…or enums Signed-off-by: Matilda Clerke <[email protected]>

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

4f544f4

7311: Add additional metrics to PeerTaskExecutor

1c268b7

Signed-off-by: Matilda Clerke <[email protected]>

7311: Add Predicate to PeerTask to check for partial success

b06f38b

Signed-off-by: Matilda Clerke <[email protected]>

7311: Fix incorrect name on isPartialSuccessTest

3c12d3d

Signed-off-by: Matilda Clerke <[email protected]>

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

b1c47ae

7311: Add partialSuccessCounter and inflightRequestGauge in PeerTaskE…

d66dd3a

…xecutor Signed-off-by: Matilda Clerke <[email protected]>

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

fa22e93

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

		/** "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

Are you sure you want to change the base?

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

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

Matilda-Clerke commented Oct 8, 2024

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