Code Analysis of Twitter’s DistributedLog

Recently, the Twitter engineering team (@TwitterEng) published an interesting library: DistributedLog, a replicated and highly efficient service to manage the logs of applications. A summary of its characteristics as found in the documentation:

• High performance, since it provides delays in the order of milliseconds with a large number of concurrent logs. It is capable of a large volume of read- and write operations per second, from thousands of clients.
• Provides durability and consistency in the data: messages are written persistent to disc and and duplicated in several copies. It prevents data loss and guarantees consistency in case of concurrency
• DistributedLog is optimized to execute in environments that are shared by independent clients (multitenant). It also guarantees isolation in the event of having to serve independent customers.

In this case, we decided to analyze the source code of DistributedLogs, because it is a recently released project (less than 2 months old), and it is still fully in development. For this, we thought the analysis of the source code can help to improve. Another important factor is the importance and popularity that this project is gaining since its publication. It is rapidly becoming a GitHub tendency within the Java world. Its principal statistics in GitHub are:

• 1788 Have marked it as favourite.
• 188 Repositories have been created as Fork from the original.
• 126 People are subscribed to its notificacions of changes.

The version we analyzed corresponds to the latest available in GitHub on 30 of may 2016 (0.3.51-SNAPSHOT). Here I will list some of the main statistics of the project to put our analysis into context:

• 399 Java files analyzed. No other programming languages were found.
• 42,529 Java lines analyzed.
• 17,641 Comment lines.
• 926 Java classes.
• 3,348 Methods were found.
• The average (cyclomatic) complexity per function is 1.9, which is a very good value.
• 0% Duplicated code: excellent!

Using the results of the Kiuwan analysis, at first glance we see that the application is being developed by a team with a lot of experience. In spite of being a medium size application, there are no large or complex files. On the contrary, the majority of the files are of a reduced size and very low complexity. This kind of file distribution normally indicates a correctly designed application, and that the files and classes have reduced and concrete responsibilities.

In the analysis of the source code based on rules, Kiuwan has found some interesting things. These are proposals to improve the code aimed at obtaining improvements in the software in these aspects:

• Making the code easier to understand for other programmers. As being an open-source application, it is interesting to iprove the readability of the code, evading the use of complex structures or instructions when there are simpler ones that can do the same.
• Improve performance of the application. Effectively, by optimizing the application at code level, in reality we only gain micro-optimizations. Nonetheless, for an application where optimum performance is expected, we should not leave any possibility unused. And a series of small differences in the end can make a really noticeable difference.
• Make the implementation by outside companies easier. In this case, the Twitter engineering team is sharing its work with the global community. The idea is that their software can be used or modified by other companies to improve the progress of the entire community. Therefore, if the application is easy to install, if it is easy to diagnose problems that are found, then the initial objectives are easier reached.

Kiuwan offers us, with an analysis based on rules, findings in more than 160 different rules. Based on the three aspects of improvement that we considered interesting for this application, as mentioned before, we have selected these 5 examples:

• There are 272 calls to System.out and System.err. There are also 4 calls to PrintStackTrace. The general recommendation is to use a log traces system (in this case, a local one), that allows us to configure the files where the errors, warnings or info’s that the application generates will be written. Apart from this, a log system allows to configure the message level, giving more flexibility to the programmer and to the people who have to use the application in production.
• 22 Times: Catching exceptions with empty bodies. There are exceptions of different types: InterruptedException, KeeperException, Exception, etc.. Even though we think that these exceptions should never occur, if they are captured it means that they are technically possible. In case that they do happen, it will be very difficult to know what is happening and to diagnose the problem. It is very recommendable, at least to add a log trace, in case these exceptions are triggered.
• 20 Private methods that are not used. Reading the code from outside, when you find a private method that is never called, the question is wether this method should have been called and is forgotten, or the call was eliminated by accident. It can also be code of a part of the application that is in development and will be used in the future, or obsolete parts that should have been eliminated. In any case, it should be revised, why these dead parts of code are there.
• 191 Methods are synchronized using the ‘synchronized’ modifier in the declaration. It is not a good programming practise to use this modifier at method level. Normally, it is more recommendable to implement synchronized code blocks inside methods. The reason is very simple. If other people modify these methods later, it will not be easy for them to realise that the entire method is synchronized. They will surely expand the method with code that is not necessary to synchronize. In the long term, the application will become more and more linear in execution, taking way less advantage of the hardware resources, and will be slower.
• 21 ‘TODO’ labels. It is good practise to periodically revise all ‘TODO’ or ‘TBD’ labels in the code. These are marks that programmers leave to remind that there are things pending implementation that may not be done yet. Some of these labels refer to changes in names or to re-factorisings of pending code, but the majority indicate that some functionality is not yet supported.

Do you believe that source code analysis can help the development team to iron out the wrinkles or to improve some aspects? From my point of view, code analysis reflects a lot of possible programming patterns and it is not always necessary to comply with all of them. As always, it depends on the nature of the application. What do you think?