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

config: warn if connmgr limits conflict with rcmgr #2527

Merged

Conversation

piersy
Copy link
Contributor

@piersy piersy commented Aug 26, 2023

closes: #1707

If the high water mark of the connection manager exceeds the total connection limit of the resource manager then a warning will be issued at node startup.

@MarcoPolo, I actually think it would be neater to pass an int to CheckLimit as opposed to what I've currently implemented.

CheckLimit(systemLimit connmgr.GetterConnLimit)

Let me know what you think

If the high water mark of the connection manager exceeds the total
connection limit of the resource manager then a warning will be issued
at node startup.
@piersy piersy force-pushed the warn-if-rcmgr-and-connmgr-limits-conflict branch from 0bcec1d to a61d291 Compare August 26, 2023 11:57
Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

I actually think it would be neater to pass an int to CheckLimit as opposed to what I've currently implemented.

That could make sense. Who would own the interface that the resourcemanager implements to get the int?

I think I prefer this model because the interface is owned by the conn manager, and the resource manager is implementing it so that it can be used to check against.

I almost would consider flipping these so that the resource manager is the one checking against other things, since it's the one that is the ultimate source of truth. But I don't think it really matters.

Besides a single comment to hopefully give a nicer warning, I think this is good. Thank you!

@piersy
Copy link
Contributor Author

piersy commented Sep 11, 2023

Hi @MarcoPolo could you elaborate on how to improve the warning message?

Currently the warnings would take the following format:

conn manager high water limit: 8, exceeds the system limit of: 4

config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
core/connmgr/manager.go Outdated Show resolved Hide resolved
p2p/net/connmgr/connmgr.go Outdated Show resolved Hide resolved
@piersy
Copy link
Contributor Author

piersy commented Sep 18, 2023

Hi @sukunrt, thanks for the review, I think I've addressed all your points.

Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

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

Thank you, @piersy

@sukunrt sukunrt merged commit 7f72151 into libp2p:master Sep 25, 2023
sukunrt added a commit that referenced this pull request Oct 18, 2023
Co-authored-by: Sukun <sukunrt@gmail.com>
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.

rcmgr: warn if limits conflict with connmgr
3 participants