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: Core
Reporter: Ran Tavory
Fix For: 0.6
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.
