cassandra-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Edward Capriolo <edlinuxg...@gmail.com>
Subject Re: Code quality, principles and rules
Date Fri, 17 Mar 2017 04:29:08 GMT
On Thu, Mar 16, 2017 at 5:18 PM, Jason Brown <jasedbrown@gmail.com> wrote:

> >> do we have plan to integrate with a dependency injection framework?
>
> No, we (the maintainers) have been pretty much against more frameworks due
> to performance reasons, overhead, and dependency management problems.
>
> On Thu, Mar 16, 2017 at 2:04 PM, Qingcun Zhou <zhouqingcun@gmail.com>
> wrote:
>
> > Since we're here, do we have plan to integrate with a dependency
> injection
> > framework like Dagger2? Otherwise it'll be difficult to write unit test
> > cases.
> >
> > On Thu, Mar 16, 2017 at 1:16 PM, Edward Capriolo <edlinuxguru@gmail.com>
> > wrote:
> >
> > > On Thu, Mar 16, 2017 at 3:10 PM, Jeff Jirsa <jjirsa@apache.org> wrote:
> > >
> > > >
> > > >
> > > > On 2017-03-16 10:32 (-0700), François Deliège <
> francois@instagram.com>
> > > > wrote:
> > > > >
> > > > > To get this started, here is an initial proposal:
> > > > >
> > > > > Principles:
> > > > >
> > > > > 1. Tests always pass.  This is the starting point. If we don't care
> > > > about test failures, then we should stop writing tests. A recurring
> > > failing
> > > > test carries no signal and is better deleted.
> > > > > 2. The code is tested.
> > > > >
> > > > > Assuming we can align on these principles, here is a proposal for
> > their
> > > > implementation.
> > > > >
> > > > > Rules:
> > > > >
> > > > > 1. Each new release passes all tests (no flakinesss).
> > > > > 2. If a patch has a failing test (test touching the same code
> path),
> > > the
> > > > code or test should be fixed prior to being accepted.
> > > > > 3. Bugs fixes should have one test that fails prior to the fix and
> > > > passes after fix.
> > > > > 4. New code should have at least 90% test coverage.
> > > > >
> > > > First I was
> > > > I agree with all of these and hope they become codified and
> followed. I
> > > > don't know anyone who believes we should be committing code that
> breaks
> > > > tests - but we should be more strict with requiring green test runs,
> > and
> > > > perhaps more strict with reverting patches that break tests (or cause
> > > them
> > > > to be flakey).
> > > >
> > > > Ed also noted on the user list [0] that certain sections of the code
> > > > itself are difficult to test because of singletons - I agree with the
> > > > suggestion that it's time to revisit CASSANDRA-7837 and
> CASSANDRA-10283
> > > >
> > > > Finally, we should also recall Jason's previous notes [1] that the
> > actual
> > > > test infrastructure available is limited - the system provided by
> > > Datastax
> > > > is not generally open to everyone (and not guaranteed to be
> permanent),
> > > and
> > > > the infrastructure currently available to the ASF is somewhat limited
> > > (much
> > > > slower, at the very least). If we require tests passing (and I agree
> > that
> > > > we should), we need to define how we're going to be testing (or how
> > we're
> > > > going to be sharing test results), because the ASF hardware isn't
> going
> > > to
> > > > be able to do dozens of dev branch dtest runs per day in its current
> > > form.
> > > >
> > > > 0: https://lists.apache.org/thread.html/
> f6f3fc6d0ad1bd54a6185ce7bd7a2f
> > > > 6f09759a02352ffc05df92eef6@%3Cuser.cassandra.apache.org%3E
> > > > 1: https://lists.apache.org/thread.html/
> 5fb8f0446ab97644100e4ef987f36e
> > > > 07f44e8dd6d38f5dc81ecb3cdd@%3Cdev.cassandra.apache.org%3E
> > > >
> > > >
> > > >
> > > Ed also noted on the user list [0] that certain sections of the code
> > itself
> > > are difficult to test because of singletons - I agree with the
> suggestion
> > > that it's time to revisit CASSANDRA-7837 and CASSANDRA-10283
> > >
> > > Thanks for the shout out!
> > >
> > > I was just looking at a patch about compaction. The patch was to
> > calculate
> > > free space correctly in case X. Compaction is not something that
> requires
> > > multiple nodes to test. The logic on the surface seems simple: find
> > tables
> > > of similar size and select them and merge them. The reality is it turns
> > out
> > > now to be that way. The coverage itself both branch and line may be
> very
> > > high, but what the code does not do is directly account for a wide
> > variety
> > > of scenarios. Without direct tests you end up with a mental
> approximation
> > > of what it does and that varies from person to person and accounts for
> > the
> > > cases that fit in your mind. For example, you personally are only
> running
> > > LevelDB inspired compaction.
> > >
> > > Being that this this is not a multi-node problem you should be able to
> re
> > > factor this heavily. Pulling everything out to a static method where
> all
> > > the parameters are arguments, or inject a lot of mocks in the current
> > code,
> > > and develop some scenario based coverage.
> > >
> > > That is how i typically "rescue" code I take over. I look at the
> > nightmare
> > > and say, "damn i am really afraid to touch this". I construct 8
> scenarios
> > > that test green. Then I force some testing into it through careful re
> > > factoring. Now, I probably know -something- about it. Now, you are
> fairly
> > > free to do a wide ranging refactor, because you at least counted for 8
> > > scenarios and you put unit test traps so that some rules are enforced.
> > (Or
> > > the person changing the code has to actively REMOVE your tests
> asserting
> > it
> > > was not or no longer is valid). Later on you (or someone else)
> __STILL__
> > > might screw the entire thing up, but at least you can now build
> forward.
> > >
> > > Anyway that patch on compaction was great and I am sure it improved
> > things.
> > > That being said it did not add any tests :). So it can easily be undone
> > by
> > > the next person who does not understand the specific issue trying to be
> > > addressed. Inline comments almost scream to me 'we need a test' not
> > > everyone believes that.
> > >
> >
> >
> >
> > --
> > Thank you & Best Regards,
> > --Simon (Qingcun) Zhou
> >
>

I don't believe dependency injection frameworks cause "overhead". For
example, if you are using spring

@Bean(initMethod=init, destroyMethod=destroy)
public Server getMeAServer( Component component) {}

@Bean(initMethod=init, destroyMethod=destroy)
public Component getMeAComponent(){}

What spring actually does is something like thisthis:

startup code
Component c = new Component();
c.init()
Server s = new Server(c);
s.init()
The default spring prototype is "singleton" aka create one of these per
bean context.

Spring also does the reverse on shutdown.

The static singleton initialization thing creates a host of problems, with
things not closing cleanly.

https://issues.apache.org/jira/browse/CASSANDRA-12844
https://issues.apache.org/jira/browse/CASSANDRA-12728

One of them I even fixed:
https://github.com/stef1927/cassandra/commits/11984-2.2

So beyond it not creating any performance problems it probably would help
in practice by forcing code into a pattern which is not singleton
initiation driven by classloader which is something very opaque and hard to
troubleshoot. Try putting a break point for example on something loaded by
a static singleton and try to reason about the order of initialization.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message