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

Enhance status with reporting maximum number of open files (sockets) #1016

Closed
wants to merge 6 commits into from

Conversation

michaelortmann
Copy link
Member

@michaelortmann michaelortmann commented Aug 13, 2020

Found by: michaelortmann
Patch by: michaelortmann
Fixes:

One-line summary:

Additional description (if needed):
While working on dns async code with threads and pipes it came to me, that enhancing .status with reporting maximum number of open files (sockets) could be useful. but i wanted the dns PR to be slim, so i copid the work for .status over to this new PR.

Test cases demonstrating functionality (if applicable):

.status
[...]
Socket table: 20/100
Memory table: 4421/8192 (54.0% full)
[...]
.status all
[...]
Socket table: 20/100
Maximum number of open files (sockets): soft limit 1024 hard limit 524288
Memory table: 4421/8192 (54.0% full)
[...]

src/chanprog.c Outdated Show resolved Hide resolved
@michaelortmann michaelortmann changed the title (Low Prio, WIP) enhance status with reporting maximum number of open files (sockets) Enhance status with reporting maximum number of open files (sockets) Sep 12, 2022
Copy link
Member

@thommey thommey left a comment

Choose a reason for hiding this comment

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

LGTM

@vanosg
Copy link
Member

vanosg commented Oct 1, 2023

Can we move this to .status all ? .status is becoming bloated.

…l parameter to distinguish between status and status all that has more detail and use it to tell maximum number of open files (sockets) only if detail requested
@michaelortmann
Copy link
Member Author

Can we move this to .status all ? .status is becoming bloated.

done. this makes this PR even more useful, because now we could easily hide more details, that are currently shown for .status and could be shown only for .status all. but this is future talk. the function tell_status() with its new parameter detail 0|1 would make it easy now.

@vanosg
Copy link
Member

vanosg commented Dec 2, 2023

I like where your head is at and appreciate you roughing in code for future work, but I think we should either implement such a change completely, or don't. Looking over how this fits in larger code, I think I'd suggest we just leave it like it is but I'm open to other thoughts or reasons to move. I think anything we move from .status to .status all can just slide right into the existing tell_verbose_status(); the existing format (I think) was created the way it was because some of these functions are called from other sections of code. @thommey thoughts?

@vanosg vanosg added this to the v1.10.0 milestone Dec 2, 2023
@michaelortmann
Copy link
Member Author

@geo: done, the status boilerplate code is now unaltered. open files/sockets are only shown for .status **all** with minimal amount of code added.

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.

3 participants