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.

Sign up for GitHub

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

Jump to bottom

nsqd: add max_channel_client_connection_count #1136

Merged
merged 1 commit into from
Feb 22, 2019
Merged

nsqd: add max_channel_client_connection_count #1136

merged 1 commit into from
Feb 22, 2019

Conversation

andyxning
Copy link
Member

@andyxning andyxning commented Feb 20, 2019 •
edited
Loading

This PR adds max_channel_client_connection_count to nsqd to limit the client connection count per channel.

This is mainly used to limit the channel consumer connections per channel.

@andyxning andyxning force-pushed the add_max_channel_client_connection_count branch 4 times, most recently from 29a78f0 to f2ef52c Compare February 20, 2019 09:04
@andyxning
Copy link
Member Author

/cc @mreiferson @ploxiln

@andyxning andyxning force-pushed the add_max_channel_client_connection_count branch 2 times, most recently from b59975d to b871a1c Compare February 20, 2019 09:29
@mreiferson mreiferson changed the title add max_channel_client_connection_count nsqd: add max_channel_client_connection_count Feb 20, 2019
mreiferson
mreiferson requested changes Feb 20, 2019
View reviewed changes
Copy link
Member

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

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

Added some comments inline, but I'd like to think on this feature in general a little bit more. Curious what @jehiah and @ploxiln think too...

apps/nsqd/nsqd.go Outdated Show resolved Hide resolved
apps/nsqd/nsqd.go Outdated Show resolved Hide resolved
@mreiferson
Copy link
Member

Thanks for the PR @andyxning!

ploxiln
ploxiln reviewed Feb 20, 2019
View reviewed changes
nsqd/protocol_v2.go Outdated Show resolved Hide resolved
@andyxning andyxning force-pushed the add_max_channel_client_connection_count branch 2 times, most recently from a34f08b to 623684c Compare February 21, 2019 02:36
@andyxning
Copy link
Member Author

All comments are addressed. PTAL. @mreiferson @ploxiln

@andyxning andyxning force-pushed the add_max_channel_client_connection_count branch from 623684c to c858fb7 Compare February 21, 2019 02:54
mreiferson
mreiferson reviewed Feb 21, 2019
View reviewed changes
nsqd/channel.go Outdated
}
c.clients[clientID] = client

return errors.New("E_TOO_MANY_CHANNEL_SUBSCRIPTIONS")
Copy link
Member

Choose a reason for hiding this comment

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

style nit-pick, I'd prefer the non-error case to be the default path and the error case to be the branch, please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

@andyxning andyxning force-pushed the add_max_channel_client_connection_count branch 2 times, most recently from 2dae48d to a277e0a Compare February 22, 2019 02:02
@andyxning
Copy link
Member Author

/cc @ploxiln @mreiferson Comments are addressed. PTAL. :)

mreiferson
mreiferson reviewed Feb 22, 2019
View reviewed changes
nsqd/protocol_v2.go Outdated Show resolved Hide resolved
@andyxning andyxning force-pushed the add_max_channel_client_connection_count branch from a277e0a to 95221cd Compare February 22, 2019 05:29
@mreiferson
Copy link
Member

LGTM, I'll leave it for @ploxiln to take a final pass.

Thanks @andyxning! 💯

@andyxning
Copy link
Member Author

andyxning commented Feb 22, 2019 •
edited
Loading

BTW, do you have a slack id for other IM id that is available for me to give you a DM about PR #1137 .
@mreiferson @ploxiln

@mreiferson
Copy link
Member

Yes, I'm mreiferson on Gophers Slack.

@andyxning
Copy link
Member Author

@ploxiln PTAL.

@ploxiln
Copy link
Member

ploxiln commented Feb 22, 2019

👍

@ploxiln ploxiln merged commit fd1cde1 into nsqio:master Feb 22, 2019
@andyxning andyxning deleted the add_max_channel_client_connection_count branch February 22, 2019 06:16
@andyxning
Copy link
Member Author

@ploxiln Does it ok to paste your IM chat name here in order to give you a DM about #1137 ?

@ploxiln
Copy link
Member

ploxiln commented Feb 22, 2019

I'm not on gophers slack ... but I am occasionally on Freenode IRC #nsq channel, using the same name, when I remember to start up my irc client :)

It's very late here at the moment so I won't be on within the next 10 hours probably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ploxiln ploxiln ploxiln left review comments

@mreiferson mreiferson mreiferson approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyxning @mreiferson @ploxiln