cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Ellis (JIRA)" <j...@apache.org>
Subject [jira] Updated: (CASSANDRA-741) Refactor for testability: Make class DatabaseDescriptor a real class with member methods, and non-static methods
Date Fri, 29 Jan 2010 20:08:34 GMT

     [ https://issues.apache.org/jira/browse/CASSANDRA-741?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Jonathan Ellis updated CASSANDRA-741:
-------------------------------------

      Component/s:     (was: Core)
                   Tools
    Fix Version/s:     (was: 0.6)
         Priority: Minor  (was: Major)

> Refactor for testability: Make class DatabaseDescriptor a real class with member methods,
and non-static methods
> ----------------------------------------------------------------------------------------------------------------
>
>                 Key: CASSANDRA-741
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-741
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: Tools
>            Reporter: Ran Tavory
>            Priority: Minor
>
> I've stumbled across this issue when working on CASSANDRA-740 (Create InProcessCassandraServer
for unit tests).
> I think the current way of how DatabaseDescriptor is implemented and used isn't great
for testability perspective, readability and extensibility, here's why - 
> The class has a giant static{} block that initializes all it's static data members. This
static block is hard wired to read a file named storage-config.xml form a system defined property
path.
>  System.getProperty("storage-config") + File.separator + "storage-conf.xml";
> This isn't great for testing. What this means is that tests need to create the folder
and put the file there (see scratch code inline the issue CASSANDRA-740). 
> Tests would rather be able to configure the class from an InputStream or something like
that. However, the fact that the class initializes itself autonomously isn't helping - a driver
of the class cannot tell it to initialize itself from a different input stream. dependency
injection is a typical solution to these cases.
> If the class had a constructor that accepts an InputStream (and an empty one that acts
at the default, reading the storage-config.xml from the file system) that would be nicer for
tests.
> It would actually be nicer not only for tests, but also for readability and extensibility
purposes - it would make all dependencies explicit (see more on this below) and it would be
possible to extend the class by inheriting and overriding some of its methods (when everything
is static it's impossible.)
> Here's a mockup of my suggestion:
> public class DatabaseDescriptor
> {
>   public DatabaseDescriptor() {
>     this(new FileInputStream(System.getProperty("storage-config") + File.separator +
"storage-conf.xml");
>   }
>   public DatabaseDescriptor(InputStream input) {
>     // read xml from input
>   }
> ...
> }
> However, implementing what's suggested above, although the change is rather trivial,
means changing all references to DatabaseDescriptor application wide... and there are more
than 200 of those...
> It usually looks like this:
> public class RingCache
> {
>     final private int port_=DatabaseDescriptor.getThriftPort();
>     final private static IPartitioner partitioner_ = DatabaseDescriptor.getPartitioner();
> The above code would have to be changed to:
> public class RingCache
> {
>   final private int port_;
>   final private static IPartitioner partitioner_;
>   public RingCache(DatabaseDescriptor descriptor) {
>     port_ = descriptor.getThriftPort();
>     partitioner_ = descriptor.getPartitioner();
> This means making the RingCache depend *explicitly* on DatabaseDescriptor since it requires
it in its constructor (and not implicitly, via a static method) so this is good for being
able to test the RingCache in a unit test and is also great for readability - no hidden dependency
loops.
> Actually, since the RingCache depends only on the thrift port and the partitioner, but
not an the entire database descriptor, it'll make even more sense to pass only the two in
the constructor. This would again make unit tests easier and code easier to understand (you
don't have to wonder "what is it in the database descriptor that I need to have in order to
make the RingCache work?")
> public class RingCache
> {
>   final private int port_;
>   final private static IPartitioner partitioner_;
>   public RingCache(int thriftPort, IPartitioner partitioner) {
>     port_ = thriftPort;
>     partitioner_ = partitioner;
> The suggested change is a code quality improvement and testability improvement and should
not affect the logic of the app.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message