besu icon indicating copy to clipboard operation
besu copied to clipboard

Record empty responses when retrying a peer task

Open fab-10 opened this issue 1 year ago • 3 comments

PR description

AbstractRetryingPeerTask has a way to understand if the result of a try is empty, but it only uses this information to discriminate if the result is a partial one, instead is very useful to also use the emptiness information to demote the peer and eventually disconnect it in case it sends too many useless responses, as done in this PR. In the making of this PR, I discovered that there were opportunities to improve the code and simplify the writing of retrying tasks, so I refactored and documented the code so that any class extending AbstractRetryingPeerTask should not set the final task result by themself, but instead implements the emptyResult and successfulResult to report the status of the request, so that the final setting of the task result is always a duty of AbstractRetryingPeerTask, removing the different approaches used before.

relates to #5415 and #5271

Tests

  • [ ] Checkpoint Sync
  • [ ] Snap Sync
  • [ ] Fast Sync
  • [ ] Full sync

fab-10 avatar May 29 '23 10:05 fab-10

  • [x] I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • [ ] I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.
  • [ ] I thought about the changelog and included a changelog update if required.
  • [x] If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

github-actions[bot] avatar May 29 '23 10:05 github-actions[bot]

SGTM. Do we demote the peer as soon as we get one empty response from it? If so, are there not cases where we receive an empty result but still want to retry with the same peer?

siladu avatar Jun 01 '23 07:06 siladu

Blocked by #6609 - without that we would disconnect peers, because we are trying peers that do not serve snap data.

macfarla avatar Apr 11 '24 02:04 macfarla

@Matilda-Clerke his could be superseded by https://github.com/hyperledger/besu/pull/7602 too

fab-10 avatar Sep 11 '24 08:09 fab-10

@fab-10 I haven't included any feedback regarding peer performance or quality in the refactor yet, so it should be possible to ensure we provide feedback for empty responses too. I'll take a good look at this PR to make sure I understand the intended behaviour.

Matilda-Clerke avatar Sep 11 '24 23:09 Matilda-Clerke