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

Re-Apply "Refactor connection.rs to make fail_with errors impossible" (double revert) #1817

Closed
wants to merge 13 commits into from

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Feb 25, 2021

TODO

  • work out why the refactor sends more errors to the syncer
  • merge these changes
  • follow-up by making this code a lot easier to maintain - split functions, map complex matches to custom enums

Motivation

We want to refactor connection.rs to make fail_with errors impossible.

Solution

Reverts #1803

The reverts will continue until moral (sync speed) improves

@dconnolly
Copy link
Contributor

Pending merge conflict fix

@dconnolly dconnolly added the A-rust Area: Updates to Rust code label Mar 4, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This PR causes multiple errors during message processing, which slows down sync a lot.

We need to work out what's going wrong with the Connection state machine, and fix it.

I have some ideas for a design that would make this code a lot easier to maintain. @yaahc let's chat when you're ready to work on this?

@teor2345
Copy link
Contributor

teor2345 commented Mar 4, 2021

I also wonder if we should implement the sync/connection tests in #1592 before merging this PR.

@mpguerra mpguerra removed this from the 2021 Sprint 5 milestone Mar 4, 2021
@teor2345 teor2345 changed the title Revert "Test: Revert "Refactor connection.rs to make fail_with errors impossible"" Re-Enable "Refactor connection.rs to make fail_with errors impossible" (double revert) Mar 17, 2021
@teor2345 teor2345 changed the title Re-Enable "Refactor connection.rs to make fail_with errors impossible" (double revert) Re-Apply "Refactor connection.rs to make fail_with errors impossible" (double revert) Mar 17, 2021
@yaahc
Copy link
Contributor Author

yaahc commented Mar 20, 2021

So status report from where I left this. The issue with this PR currently is that it causes the sync component to restart syncing in response to mixed inventory errors. The previous version seems to somehow not error out in a way that restarts syncing when it encounters these same errors. The next steps are to figure out how the current version handles mixed inventory errors, and see how that differs from the refactor.

@teor2345
Copy link
Contributor

teor2345 commented Mar 22, 2021

It's also worth noting that these errors have an outsized impact because:

It's weird that the current connection code appears to be dropping some of these errors - or at least responding to them differently.

But regardless of the fragility of this code and its context, we still need to make sure the refactor performs as well as the previous code.

@teor2345
Copy link
Contributor

We've decided to close this PR for now, see ticket #1610 for next steps.

@teor2345 teor2345 closed this Mar 31, 2021
@teor2345 teor2345 deleted the revert-1803-revert-1721-jane/fail-with branch March 21, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants