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

nsqd: expose memory stats under /stats #874

Merged
merged 1 commit into from
Apr 6, 2017
Merged

Conversation

sparklxb
Copy link
Contributor

No description provided.

@mreiferson
Copy link
Member

@sparklxb this is great! thank you for the contribution 😍

I have one request — can we consolidate this new getMemStats code with the similar code being used https://github.com/sparklxb/nsq/blob/5a888824fd0b762594575b3525c7abd1df9bb2ed/nsqd/statsd.go#L118-L138?

Thanks!

@mreiferson
Copy link
Member

ping #846

@mreiferson mreiferson changed the title resolve #846: nsqd: expose memory stats under /stats nsqd: expose memory stats under /stats Mar 31, 2017
@sparklxb
Copy link
Contributor Author

Done

@mreiferson
Copy link
Member

fantastic, mind squashing down to one commit and removing the resolve #846: prefix?

@ploxiln
Copy link
Member

ploxiln commented Mar 31, 2017

Just as an aside - the github "Squash and merge" mode works, I've used it once or twice. It lets you modify the commit title and message, then it puts the new commit on top of the target branch (e.g. master) without any merge commit.

Also interestingly, it marks the pull request as closed and merged, but without changing the pull-request branch, so the new commit is only in the target branch and the pull request still shows the old commits.

@mreiferson
Copy link
Member

Yea, I know, I wish there were an option to do all of that with the merge commit, which I've gotten used to and found valuable when browsing history/lineage.

@jehiah
Copy link
Member

jehiah commented Mar 31, 2017

ditto. for better or worse i like the merge commit.

@sparklxb
Copy link
Contributor Author

sparklxb commented Apr 1, 2017

@mreiferson mreiferson merged commit ad70e25 into nsqio:master Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants