Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Queue pending requests when all peers are busy #1331

Merged
merged 35 commits into from
May 2, 2019

Conversation

ajsutton
Copy link
Contributor

PR description

Modify how idle peers are found so that instead of picking one and then separately sending the request, that's done in a single atomic operation. This ensures that two threads don't select the same peer and both send requests to it, potentially breaching the outstanding request limit.

Additionally, if all suitable peers are busy queue the request and perform it when a peer is available instead of throwing NoAvailablePeersException. This avoids aborting pipelines when there is a lot of contention for peers and provides a fairer sharing of peers between the various threads that need to request data.

ajsutton and others added 21 commits April 26, 2019 14:46
… include the time queueing for an available peer.
…ing with pending peer requests. Send all peer requests through EthPeers, even if a specific peer has been requested.
…ps (PegaSysEng#1318)

* Allow private contract invocations in multiple privacy groups
Store the header for the current chain head and total difficulty to avoid RocksDB lookups when requesting those common values.

Also uses that cache to avoid a database lookup when checking if a block has already been imported if the block's parent is the current chain head.
* combine bootnodes and staticNodes and pass the combined collection when building permissioning config; renamed error code that specifically called out bootnodes
… the same number of outstanding requests. Ensures we cycle round all our peers to avoid sybil attacks.
@ajsutton ajsutton marked this pull request as ready for review April 30, 2019 21:56
Copy link
Contributor

@mbaxter mbaxter left a comment

Choose a reason for hiding this comment

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

Looks good!

* @param onError handler for when there is no peer with sufficient height or the request fails to
* send
*/
public void thenEither(
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) From javascript, I would expect this to be called 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.

Works for me.

}

private void useRequestSlot(final EthPeer peer) throws PeerNotConnected {
peer.getNodeData(emptyList());
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like getNodeData() should throw an exception here. For now, it's probably worth at least sending a non-empty list to this method in order to future-proof these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ajsutton ajsutton merged commit 76b1fa5 into PegaSysEng:master May 2, 2019
@ajsutton ajsutton deleted the claim-peers branch May 2, 2019 21:13
notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 4, 2019
Provides better control over the maximum number of concurrent requests to a peer.
Gives a fairer allocation of peers to requests being made.
Avoids a task failing because all peers are busy which then introduces a delay before the request is retried.
notlesh pushed a commit to notlesh/pantheon that referenced this pull request May 14, 2019
Provides better control over the maximum number of concurrent requests to a peer.
Gives a fairer allocation of peers to requests being made.
Avoids a task failing because all peers are busy which then introduces a delay before the request is retried.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants