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

Log any exception that kills a Runnable.run #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jdanbrown
Copy link
Contributor

I haven't actually caught any bugs with this yet, but I've been seeing some intermittent problems around expired zk sessions that I'm hoping to diagnose. (I'm coming to suspect that it's due to bugs in the twitter zk watcher logic.)

And more generally, threads that die silently freak me out.

Note that this logging commit depends on and includes the gauged balancing policy commit from #10.

@cscotta
Copy link
Contributor

cscotta commented Oct 24, 2012

Hey Dan,

Sorry for the delay in getting back to you here. Really appreciate this PR and I'd like to merge it. It's been a bit since I've had time to focus on Ordasity proper and want to apologize for the delay in attention to this.

The exception logging for all Runnables is great.

I like the idea of gauge-based metering as well (we have a couple memory-bound applications for which this would be excellent, once we have a better way of accurately measuring that).

The only comment I have is on the tuple pattern matching to determine whether or not the combination of BalancingPolicy and Listener is valid. Would you mind if I moved this out of the startWork method and instead brought it up to the initialization level? E.g., throw the exception if the user attempts to initialize the Cluster object with an invalid combination, rather than waiting until startWork() is called on a work unit to throw. Seems best to catch that up front, and nice to move the logic out of the startWork() call.

If you're down with that, I'm happy to merge this PR and make the change mentioned above myself.

Thanks!

– Scott

@jdanbrown
Copy link
Contributor Author

Hey Scott, no problem with the delays. Ordasity's been cruising quite nicely. :)

Go ahead with refactoring the BalancingPolicy+Listener stuff to fail faster, I think that's a good improvement. I did it the way it is now just because it was easy and not intrusive.

Feel free to pepper me with Q's here or on twitter. And eventually, when you have some more time, I'd enjoy picking your brain about #18.

Dan

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.

2 participants