[CASSANDRA-741] Refactor for testability: Make class DatabaseDescriptor a real class with member methods, and non-static methods

• Type: Improvement
• Status: Resolved
• Priority: Low
• Resolution: Won't Fix
• Component/s: Legacy/Tools
• Assignee: Unassigned
• Reporter: Ran Tavory
• Created: 26/Jan/10 07:42
• Updated: 16/Apr/19 09:33
• Resolved: 29/Jan/10 20:25

Description

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.