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

Update overlay survey script with lessons learned during testnet run #4358

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

bboston7
Copy link
Contributor

Description

This change makes a few tweaks to the overlay survey script to fix some small things I noticed after running it on testnet:

  • Changes the script's end condition to depend only on responses, and not requests. Without this it was possible for the survey script to run for the full duration of the collecting phase (2 hours) if a node with more than 25 peers stopped responding after the surveyor received the first set of peers.
  • Downgrades the severity of "node already in backlog" messages from error to debug. This is an expected condition that I simply forgot to special-case before.
  • Modifies the simulator to occasionally return "node already in backlog" messages to test the script against that case.
  • Adds a --fast option to the simulate mode that skips any sleep calls. This makes the script much nicer to test.
  • Fixes naming of graphml fields to match JSON result fields.
    • I did most of this in the V2 script update, but missed a couple spots.
    • Most of this change is in the simulator to support the new field names.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@bboston7 bboston7 requested a review from SirTyson June 21, 2024 18:23
Copy link
Contributor

@marta-lokhova marta-lokhova left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few questions. Looks like the PR needs to be rebased also to re-trigger CI.

@marta-lokhova
Copy link
Contributor

any update on this change? would be nice to land it in v21.2.0 (starts today), so we can use new-style script for pubnet.

This change makes a few tweaks to the overlay survey script to fix some
small things I noticed after running it on testnet:

* Changes the scripts end condition to depend only on responses, and not
  requests. Without this it was possible for the survey script to run
  for the full duration of the collecting phase (2 hours) if a node with
  more than 25 peers stopped responding after the surveyor recieved the
  first set of peers.
* Downgrades the severity of "node already in backlog" messages from
  `error` to `debug`. This is an expected condition that I simply forgot
  to special-case before.
* Modifies the simulator to occasionally return "node already in
  backlog" messages to test the script against that case.
* Adds a `--fast` option to the `simulate` mode that skips any `sleep`
  calls. This makes the script much nicer to test.
* Fixes naming of graphml fields to match JSON result fields.
  * I did most of this in the V2 script update, but missed a couple
    spots.
  * Most of this change is in the simulator to support the new field
    names.
@bboston7
Copy link
Contributor Author

bboston7 commented Jul 8, 2024

any update on this change?

I just responded to your comments and rebased the change

@marta-lokhova marta-lokhova added this pull request to the merge queue Jul 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 8, 2024
@marta-lokhova marta-lokhova added this pull request to the merge queue Jul 8, 2024
Merged via the queue into stellar:master with commit d78f48e Jul 8, 2024
14 checks passed
@bboston7 bboston7 deleted the survey-script-tweaks branch July 8, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants