I'd like to think that if someone refactors existing code, making it more testable (with tests, of course) it should be acceptable on it's own merit. In fact, in my opinion it sometimes makes more sense to do these types of refactorings for the sole purpose of improving stability and testability as opposed to mixing them with features.
You referenced the issue I fixed in one of the early emails. The fix itself was a couple lines of code. Refactoring the codebase to make it testable would have been a huge effort. I wish I had time to do it. I created CASSANDRA-13007 as a follow up with the intent of working on compaction from a purely architectural standpoint. I think this type of thing should be done throughout the codebase. Removing the singletons is a good first step, my vote is we just rip off the bandaid, do it, and move forward. On Fri, Mar 17, 2017 at 2:20 PM Edward Capriolo <edlinuxg...@gmail.com> wrote: > On Fri, Mar 17, 2017 at 2:31 PM, Jason Brown <jasedbr...@gmail.com> wrote: > > > To François's point about code coverage for new code, I think this makes > a > > lot of sense wrt large features (like the current work on > 8457/12229/9754). > > It's much simpler to (mentally, at least) isolate those changed sections > > and it'll show up better in a code coverage report. With small patches, > > that might be harder to achieve - however, as the patch should come with > > *some* tests (unless it's a truly trivial patch), it might just work > itself > > out. > > > > On Fri, Mar 17, 2017 at 11:19 AM, Jason Brown <jasedbr...@gmail.com> > > wrote: > > > > > As someone who spent a lot of time looking at the singletons topic in > the > > > past, Blake brings a great perspective here. Figuring out and > > communicating > > > how best to test with the system we have (and of course incrementally > > > making that system easier to work with/test) seems like an achievable > > goal. > > > > > > On Fri, Mar 17, 2017 at 10:17 AM, Edward Capriolo < > edlinuxg...@gmail.com > > > > > > wrote: > > > > > >> On Fri, Mar 17, 2017 at 12:33 PM, Blake Eggleston < > beggles...@apple.com > > > > > >> wrote: > > >> > > >> > I think we’re getting a little ahead of ourselves talking about DI > > >> > frameworks. Before that even becomes something worth talking about, > > we’d > > >> > need to have made serious progress on un-spaghettifying Cassandra in > > the > > >> > first place. It’s an extremely tall order. Adding a DI framework > right > > >> now > > >> > would be like throwing gasoline on a raging tire fire. > > >> > > > >> > Removing singletons seems to come up every 6-12 months, and usually > > >> > abandoned once people figure out how difficult they are to remove > > >> properly. > > >> > I do think removing them *should* be a long term goal, but we really > > >> need > > >> > something more immediately actionable. Otherwise, nothing’s going to > > >> > happen, and we’ll be having this discussion again in a year or so > when > > >> > everyone’s angry that Cassandra 5.0 still isn’t ready for > production, > > a > > >> > year after it’s release. > > >> > > > >> > That said, the reason singletons regularly get brought up is because > > >> doing > > >> > extensive testing of anything in Cassandra is pretty much > impossible, > > >> since > > >> > the code is basically this big web of interconnected global state. > > >> Testing > > >> > anything in isolation can’t be done, which, for a distributed > > database, > > >> is > > >> > crazy. It’s a chronic problem that handicaps our ability to release > a > > >> > stable database. > > >> > > > >> > At this point, I think a more pragmatic approach would be to draft > and > > >> > enforce some coding standards that can be applied in day to day > > >> development > > >> > that drive incremental improvement of the testing and testability of > > the > > >> > project. What should be tested, how it should be tested. How to > write > > >> new > > >> > code that talks to the rest of Cassandra and is testable. How to fix > > >> bugs > > >> > in old code in a way that’s testable. We should also have some > > >> guidelines > > >> > around refactoring the wildly untested sections, how to get started, > > >> what > > >> > to do, what not to do, etc. > > >> > > > >> > Thoughts? > > >> > > >> > > >> To make the conversation practical. There is one class I personally > > really > > >> want to refactor so it can be tested: > > >> > > >> https://github.com/apache/cassandra/blob/trunk/src/java/org/ > > >> apache/cassandra/net/OutboundTcpConnection.java > > >> > > >> There is little coverage here. Questions like: > > >> what errors cause the connection to restart? > > >> when are undropable messages are dropped? > > >> what happens when the queue fills up? > > >> Infamous throw new AssertionError(ex); (which probably bubble up to > > >> nowhere) > > >> what does the COALESCED strategy do in case XYZ. > > >> A nifty label (wow a label you just never see those much!) > > >> outer: > > >> while (!isStopped) > > >> > > >> Comments to jira's that probably are not explicitly tested: > > >> // If we haven't retried this message yet, put it back on the queue to > > >> retry after re-connecting. > > >> // See CASSANDRA-5393 and CASSANDRA-12192. > > >> > > >> If I were to undertake this cleanup, would there actually be support? > IE > > >> if > > >> this going to turn into an "it aint broken. don't fix it thing" or a > "we > > >> don't want to change stuff just to add tests" . Like will someone > pledge > > >> to > > >> agree its kinda wonky and merge the effort in < 1 years time? > > >> > > > > > > > > > > So ...:) If open a ticket to refactor OutboundTcpConnection.java to do > specific unit testing and possibly even pull things out to the point that I > can actually open a socket and to an end to end test will you/anyone > support that? (it sounds like your saying I must/should make a large > feature to add a test) >
