can now disconnect from cassandra in clients and clusters

[bmuller]

When testing other libs that use silverberg, it's essential that connections can be closed so that the reactor is left in a clean state after each test run. I moved the disconnect method to the main CQLClient class and added a disconnect method to the round robin cluster as well.

Wrote new test case. Lint / tests pass.

[stretchservers]

Can one of the admins verify this patch?

[dreid]

Hi @bmuller,

Thanks for your contribution and your interest in silverberg. We now use travis-ci for continuous integration (instead of the jenkins instance that you couldn't see.)

Could you rebase and update this pull request and #19?

Thanks.

[bmuller]

done

On 7/11/13 7:11 PM, cyli wrote:

Hi @bmuller https://github.com/bmuller!

Thank you for this change! Would you mind also moving the test for
disconnect from the MockTestingClientTests to the MockClientTests
instead, since the client also now supports disconnecting?

Thanks again!

—
Reply to this email directly or view it on GitHub
#18 (comment).

[cyli]

Thanks for the quick response! The testing comment here can probably also be removed.

[wirehead]

So, sorry if I'm being annoying and bikeshedding, but I was thinking that it would be most clear to have the doc comments say "Cassandara and Silverberg don't require you to close the connection before exiting, but you might want to for cases of testing, constrained resources, etc" instead of "mostly used for testing" or nothing at all.

Yeah, I'm bikeshedding. Sorry. :(

[bmuller]

@cyli and @wirehead - I think I've been pretty accommodating so far. However, if your comments take longer to write then it would take to just make the change yourself - the message you send is that you're more interested in pedantry than getting stuff done.

You're killing my desire to contribute with a thousand paper cuts of trivial requests. Just make the little changes yourself and accept the PR already.

[cyli]

@bmuller Indeed, thank you again for your generous contributions and your prompt responses! I apologize about my confusion about the use of disconnect, which resulted in an extra change for you. I should have first double-checked with @wirehead first before making the comment, as I did not have a good grasp of how silverberg cleans up connections.

I have also been remiss in not further elaborating on our coding and testing standards in the docs, but in general we try to aim for 100% test coverage of changes made in every code request, and not just as documented by the coverage tool (which is a good baseline but does not say anything about the quality of the tests or correctness of behavior the tests are verifying).

When the disconnect function was moved, its test was no longer valid where it was. If the disconnect function had been moved back, tests would still pass, therefore the new functionality (that the CQLClient, as opposed to the TestCQLClient, can disconnect) is untested.

If you would like this change made, I can certainly make the requisite updates your contributions so far to comply with our general process. The best way would probably be for me to pull your changes into my own fork that I have push access to, make the test and comment changes, and submit a new request.

Would that be satisfactory to you?

[bmuller]

@cyli feel free to make any changes to any of my PR's at any point to fix whatever issues may prevent acceptance. My only goal is to get the fixes/features incorporated into master - and I don't care at all even if those changes are squashed into one of your commits.

[cyli]

@bmuller Thank you for your understanding and flexibility - that will make my life a lot easier than experimenting with how to fork a fork and then make a pull request back to the original. :)

[bmuller]

Sure - no worries.

[cyli]

@bmuller Created a new pull request with the requisite changes, and will merge once it passes review. Closing this PR.

[bmuller]

Excellent - thanks so much!

[cyli]

@bmuller The changes have been merged in! Thanks again for your patience.

[bmuller]

No problem - thanks for merging. In the future - feel free to mangle/squash/manipulate/recreate/etc any of my PR's to get them merged (especially if there are only minor changes/edits necessary). If there's anything major - like bugs, structure issues, etc - that would/should require major changes on my part - then I can definitely handle those myself.

Thanks so much!