[
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.
