Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add tube enqueue dequeue counts #90

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants

dsmith commented Jan 26, 2012

Wanted to get some thoughts on generating put/reserve counts on a per tube basis. Having the global connection count is nice but I'd like to monitor publish and consumption rates for specific tubes.

Owner

kr commented Jan 27, 2012

Looks useful. Can you merge these four commits into one
and rebase onto the latest master?

dsmith commented Jan 27, 2012

Commits have been condensed to one and the branch has been rebased against master.

Owner

kr commented Jan 27, 2012

Hey, this looks great! Just a couple small things that would be
great to fix. I made comments inline on the diff. After that it
should be good for me to pull into master.

dsmith commented Jan 31, 2012

I've made the changes in separate commits. Would you prefer them to be squashed into one commit?

Owner

kr commented Jan 31, 2012

Yes, one commit would be best. Thanks so much.

dsmith commented Jan 31, 2012

They will has been done :)

Owner

kr commented Feb 1, 2012

Did you run the tests? I'm trying out your patch and it looks like
you need to update a couple of the unit tests, at least the files
sh-tests/pause-tube.expected and
sh-tests/stats_tube.expected. Sorry I didn't notice that
before.

dsmith commented Feb 6, 2012

I ran the tests on the master branch and still found the same tests that were failing in my branch:

sh-tests/pause-tube.commands
46,47c46,47
< current-jobs-urgent: 1
< current-jobs-ready: 1
---
> current-jobs-urgent: 0
> current-jobs-ready: 0
49c49
< current-jobs-delayed: 0
---
> current-jobs-delayed: 1
60c60
< OK 143
---
> OK 145
64c64
< state: ready
---
> state: delayed
integ-test.c:128: test: diffst == 0
integ-test.c:128: was 256

17 tests; 1 failures; 0 errors.
make: *** [check-ct] Error 1

I've merge master into my branch and the same test is still failing.

Owner

kr commented May 31, 2012

The tests have recently been reorganized and should be less
susceptible to races such as the one you ran into above.

It shouldn't be too hard to update this patch to work with the
new tests, if you're still interested.

Owner

kr commented Feb 1, 2013

This pull request is pretty stale now. Feel free to reopen if you're
interested in reviving it. :)

@kr kr closed this Feb 1, 2013

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