Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7311: add peertask foundation code #7628

Merged
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
4b80016
7311: Add PeerTask system for use in future PRs
Matilda-Clerke Sep 17, 2024
a8d5a9f
7311: Clean up some warnings
Matilda-Clerke Sep 17, 2024
e4be5c0
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 18, 2024
5859444
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 19, 2024
c335cbe
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 20, 2024
08c66fd
7311: Reduce timeout in PeerTaskRequestSender to 5s
Matilda-Clerke Sep 20, 2024
049cae2
7311: Refactor PeerManager to be an interface
Matilda-Clerke Sep 20, 2024
5afba63
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 20, 2024
f2ac53e
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 23, 2024
e901fdf
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 24, 2024
6e349e1
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 25, 2024
ad86ae6
7311: Rename PeerManager to PeerSelector
Matilda-Clerke Sep 25, 2024
38f04ab
7311: Reword PeerSelector javadoc to avoid implementation details
Matilda-Clerke Sep 25, 2024
6de3fb3
7311: Use ConcurrentHashMap in DefaultPeerSelector
Matilda-Clerke Sep 25, 2024
da9cd43
7311: Reword trace log in DefaultPeerSelector
Matilda-Clerke Sep 25, 2024
ce7d245
7311: Remove unused imports
Matilda-Clerke Sep 25, 2024
c9eb22e
7311: Use a 1 second delay between retries in PeerTaskExecutor to mat…
Matilda-Clerke Sep 25, 2024
e2fda73
7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest
Matilda-Clerke Sep 25, 2024
608fece
7311: Add testGetPeerButNoPeerMatchesFilter to DefaultPeerSelectorTest
Matilda-Clerke Sep 25, 2024
2d07800
7311: spotless
Matilda-Clerke Sep 25, 2024
ad26297
7311: Fix MetricsAcceptanceTest
Matilda-Clerke Sep 20, 2024
96c8030
7311: Fix MetricsAcceptanceTest
Matilda-Clerke Sep 20, 2024
b0f2ed0
7311: Modify PeerTaskExecutor metric to include response time from peer
Matilda-Clerke Sep 26, 2024
598b519
7311: Use SubProtocol instead of subprotocol name string in PeerTask
Matilda-Clerke Sep 26, 2024
bc25b16
7311: rename timing context to ignored to prevent intellij warnings
Matilda-Clerke Sep 26, 2024
e31bb70
7311: Use constants for number of retries
Matilda-Clerke Sep 26, 2024
41923d3
7311: Convert PeerTaskExecutorResult to a record
Matilda-Clerke Sep 26, 2024
720f94e
7311: Rename PeerTaskBehavior to PeerTaskRetryBehavior
Matilda-Clerke Sep 29, 2024
7d845b3
7311: Move peer selection logic to PeerSelector
Matilda-Clerke Sep 30, 2024
50c26f1
7311: spotless
Matilda-Clerke Sep 30, 2024
b7c0c95
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 30, 2024
a81855d
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Sep 30, 2024
8718102
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 3, 2024
e63f473
7311: Make changes as discussed in walkthrough meeting
Matilda-Clerke Oct 3, 2024
d1847f2
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 4, 2024
6d2cb95
7311: Rename getPeerTaskBehavior to getPeerTaskRetryBehavior
Matilda-Clerke Oct 6, 2024
d84520a
7311: Rename getPeerTaskBehavior to getPeerTaskRetryBehavior
Matilda-Clerke Oct 6, 2024
77ed748
Merge remote-tracking branch 'origin/7311-add-peertask-foundation-cod…
Matilda-Clerke Oct 6, 2024
0896e31
7311: Rework PeerTaskExecutor retry system to be 0-based
Matilda-Clerke Oct 6, 2024
5f924c4
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 6, 2024
2865625
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 7, 2024
82cedb0
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 7, 2024
bdd96ba
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 8, 2024
c047f42
7311: Remove unused async methods in PeerTaskExecutor
Matilda-Clerke Oct 8, 2024
5aa6b0b
7311: Return Optional<EthPeer> in PeerSelector.getPeer and utilise ex…
Matilda-Clerke Oct 8, 2024
8becdb3
7311: Redo getPeer again to include hasAvailableRequestCapacity check
Matilda-Clerke Oct 8, 2024
8186a77
7311: Rework getPeer again to use LEAST_TO_MOST_BUSY comparator
Matilda-Clerke Oct 8, 2024
37b0ec2
7311: Import PeerNotConnected class instead of using fully qualified …
Matilda-Clerke Oct 8, 2024
545fd5c
7311: Change to specifying retry counts in PeerTask instead of behavi…
Matilda-Clerke Oct 9, 2024
4f544f4
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 9, 2024
1c268b7
7311: Add additional metrics to PeerTaskExecutor
Matilda-Clerke Oct 10, 2024
b06f38b
7311: Add Predicate to PeerTask to check for partial success
Matilda-Clerke Oct 10, 2024
3c12d3d
7311: Fix incorrect name on isPartialSuccessTest
Matilda-Clerke Oct 10, 2024
b1c47ae
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 10, 2024
d66dd3a
7311: Add partialSuccessCounter and inflightRequestGauge in PeerTaskE…
Matilda-Clerke Oct 11, 2024
fa22e93
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 11, 2024
a3f5d4a
7311: Also filter by whether a peer is fully validated
Matilda-Clerke Oct 11, 2024
3a68980
7311: Fix up inflight requests gauge in PeerTaskExecutor
Matilda-Clerke Oct 11, 2024
c422bc5
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 11, 2024
56c1f9d
7311: Update plugin api hash
Matilda-Clerke Oct 11, 2024
e733452
7311: Add javadoc to LabelledGauge.isLabelsObserved
Matilda-Clerke Oct 13, 2024
b3a252b
7311: Update plugin-api hash
Matilda-Clerke Oct 13, 2024
4f9cf52
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 13, 2024
3c96eba
7311: Update changelog
Matilda-Clerke Oct 13, 2024
7daf30f
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 15, 2024
44fd3a8
7311: Use taskName instead of className for labelNames
Matilda-Clerke Oct 16, 2024
ac1c4ed
7311: Use snake_case for metric names
Matilda-Clerke Oct 16, 2024
7503535
7311: Use _total metric name suffix
Matilda-Clerke Oct 16, 2024
ed25941
7311: rework partial success handling
Matilda-Clerke Oct 17, 2024
5a79636
7311: Add default implementation to LabelledGauge.isLabelsObserved
Matilda-Clerke Oct 17, 2024
b075a91
7311: Fix broken unit test
Matilda-Clerke Oct 17, 2024
74995bb
Merge branch 'main' into 7311-add-peertask-foundation-code
Matilda-Clerke Oct 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.manager.peertask;

import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
import org.hyperledger.besu.ethereum.p2p.peers.PeerId;

import java.util.Comparator;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Predicate;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* This is a simple PeerSelector implementation that can be used the default implementation in most
* situations
*/
public class DefaultPeerSelector implements PeerSelector {
private static final Logger LOG = LoggerFactory.getLogger(DefaultPeerSelector.class);

private final Map<PeerId, EthPeer> ethPeersByPeerId = new ConcurrentHashMap<>();

/**
* Gets the highest reputation peer matching the supplied filter
*
* @param filter a filter to match prospective peers with
* @return the highest reputation peer matching the supplies filter
* @throws NoAvailablePeerException If there are no suitable peers
*/
@Override
public EthPeer getPeer(final Predicate<EthPeer> filter) throws NoAvailablePeerException {
LOG.trace("Finding peer from pool of {} peers", ethPeersByPeerId.size());
return ethPeersByPeerId.values().stream()
.filter(filter)
.max(Comparator.naturalOrder())
.orElseThrow(NoAvailablePeerException::new);
}

@Override
public Optional<EthPeer> getPeerByPeerId(final PeerId peerId) {
return Optional.ofNullable(ethPeersByPeerId.get(peerId));
}

@Override
public void addPeer(final EthPeer ethPeer) {
ethPeersByPeerId.put(ethPeer.getConnection().getPeer(), ethPeer);
}

@Override
public void removePeer(final PeerId peerId) {
ethPeersByPeerId.remove(peerId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.manager.peertask;

public class InvalidPeerTaskResponseException extends Exception {

public InvalidPeerTaskResponseException() {
super();
}

public InvalidPeerTaskResponseException(final Throwable cause) {
super(cause);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.manager.peertask;

public class NoAvailablePeerException extends Exception {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.manager.peertask;

import org.hyperledger.besu.ethereum.eth.manager.EthPeer;
import org.hyperledger.besu.ethereum.p2p.peers.PeerId;

import java.util.Optional;
import java.util.function.Predicate;

/** Selects the EthPeers for the PeerTaskExecutor */
public interface PeerSelector {

/**
* Gets a peer matching the supplied filter
*
* @param filter a filter to match prospective peers with
* @return a peer matching the supplied filter
* @throws NoAvailablePeerException If there are no suitable peers
*/
EthPeer getPeer(final Predicate<EthPeer> filter) throws NoAvailablePeerException;

/**
* Attempts to get the EthPeer identified by peerId
*
* @param peerId the peerId of the desired EthPeer
* @return An Optional\<EthPeer\> containing the EthPeer identified by peerId if present in the
* PeerSelector, or empty otherwise
*/
Optional<EthPeer> getPeerByPeerId(final PeerId peerId);

/**
* Add the supplied EthPeer to the PeerSelector
*
* @param ethPeer the EthPeer to be added to the PeerSelector
*/
void addPeer(final EthPeer ethPeer);

/**
* Remove the EthPeer identified by peerId from the PeerSelector
*
* @param peerId the PeerId of the EthPeer to be removed from the PeerSelector
*/
void removePeer(final PeerId peerId);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.manager.peertask;

import org.hyperledger.besu.ethereum.p2p.rlpx.wire.MessageData;

import java.util.Collection;

/**
* Represents a task to be executed on an EthPeer by the PeerTaskExecutor
*
* @param <T> The type of the result of this PeerTask
*/
public interface PeerTask<T> {
/**
* Returns the SubProtocol used for this PeerTask
*
* @return the SubProtocol used for this PeerTask
*/
String getSubProtocol();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Gets the minimum required block number for a peer to have to successfully execute this task
*
* @return the minimum required block number for a peer to have to successfully execute this task
*/
long getRequiredBlockNumber();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might need nice to combine these into a "prerequisites" object. Feels like details leaking into the interface

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a musing...what if we pass a peer into the task's prerequisite filter?

e.g. "here's my set of peers sorted by rep, choose the first one that matches my task's criteria."

Currently feels like peer selection logic is spread out across the Manager, Executor and Task


/**
* Gets the request data to send to the EthPeer
*
* @return the request data to send to the EthPeer
*/
MessageData getRequestMessage();

/**
* Parses the MessageData response from the EthPeer
*
* @param messageData the response MessageData to be parsed
* @return a T built from the response MessageData
* @throws InvalidPeerTaskResponseException if the response messageData is invalid
*/
T parseResponse(MessageData messageData) throws InvalidPeerTaskResponseException;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep true

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright contributors to Hyperledger Besu.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
*/
package org.hyperledger.besu.ethereum.eth.manager.peertask;

public enum PeerTaskBehavior {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public enum PeerTaskBehavior {
public enum PeerTaskRetryBehavior {

RETRY_WITH_SAME_PEER,
RETRY_WITH_OTHER_PEERS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, that makes sense

}
Loading
Loading