Skip to content

Conversation

@MarcoPolo
Copy link
Collaborator

We see flakiness in CI because a client will dial with concurrency 8 but the limit on the server is 4 so we get rcmgr failures. We notice this when we have a lot of addrs to dial.

Fixes #1816
Fixes #1765
Fixes #1730
Fixes #1668

(will rebase to master after #1881 is merged)

@marten-seemann
Copy link
Contributor

(will rebase to master after #1881 is merged)

Why not target master from this PR? Otherwise this commit will be lost when we squash #1881.

@MarcoPolo MarcoPolo force-pushed the marco/synchronize-limits-with-dials branch from 0b7bfc7 to 65a2a55 Compare November 17, 2022 05:22
@MarcoPolo
Copy link
Collaborator Author

(will rebase to master after #1881 is merged)

Why not target master from this PR? Otherwise this commit will be lost when we squash #1881.

To make it easier to review. I'll merge this to master so we'll keep this commit. I should have said target master after #1881

@MarcoPolo MarcoPolo changed the base branch from marco/quic-v1 to master November 17, 2022 18:01
@MarcoPolo MarcoPolo force-pushed the marco/synchronize-limits-with-dials branch from 65a2a55 to 92d131b Compare November 17, 2022 18:02
@MarcoPolo
Copy link
Collaborator Author

Oh I see what you meant. This PR didn't build on the quic-v1 branch so there was no point in targeting that branch. I just defaulted to stacking it since it came from that branch, but I should have just based this on master from the start.

@marten-seemann marten-seemann changed the title Synchronize the concurrent outbound dials with limits swarm / rcmgr: synchronize the concurrent outbound dials with limits Nov 21, 2022
@MarcoPolo MarcoPolo force-pushed the marco/synchronize-limits-with-dials branch from 54432e3 to aef3657 Compare November 22, 2022 21:50
@MarcoPolo
Copy link
Collaborator Author

@marten-seemann and I talked about this and agreed to bump the limit up to 8 for now. This will be handled better when we have smart dialing.

@MarcoPolo MarcoPolo mentioned this pull request Nov 22, 2022
34 tasks

PeerBaseLimit: BaseLimit{
ConnsInbound: 4,
ConnsInbound: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining why this is 8 for now, and that smart dialing will allow us to reduce this back to 4?

@MarcoPolo MarcoPolo merged commit 9c9122d into master Nov 22, 2022
@p-shahi p-shahi mentioned this pull request Nov 22, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

3 participants