Code Review 1

In the first round of code reviews, each team will present a portion of its design in class and other students will comment on the design and offer suggestions for improvement. Each team will also receive written code reviews on their project, including partial reviews from other students in the class and a full review from me. After the discussions in class, I will meet individually with each team to go over my code review in detail.

The rest of this document describes how we will conduct the code reviews in class.

Presenters

To start off each code review, the presenting team will spend 10-15 minutes to address the following issues:

• For each of the classes under discussion, describe the key idea(s) behind that class. In particular, describe the information and/or design decisions that are hidden within that class. Note that you will be discussing one particular aspect of your project; there will not be enough time for you to present all of your classes.
• If you considered alternate designs, say what the alternatives were and how you chose between them.
• Go over (briefly) a few key methods, discussing the API for each briefly, in order to give the audience a feel for what are the pieces within the class.
• Walk through an example of usage, such as the life cycle of a remote procedure call or the processing of a particular request.
• Talk briefly about what you found hard and easy in designing your system.
• What are the elements of your design that you are happiest about? What are its weaknesses? Ideally, you will already be aware of all of the issues discovered by reviewers.

You should prepare slides for your presentation. Remember the overall goal is abstraction: finding a simple way to think about something that is internally complicated. Try to find a simple way to explain your design, so that even people who have not read the code can easily get the basic idea; think about what are the most important elements of your design. Of course, if there are tricky elements, you will also need to mention those as well (don't pretend something is simple when it really isn't).

Reviewers

Each project will be reviewed by four students from other projects, according to the table below. Reviewers must read over the relevant code before the given class and prepare a written code review on GitHub. To do this, go to the project in GitHub, click on the "Pull requests" tab, click on the "Project1" pull request, and click on the "Files changed" tab. Click on a line on the right side to enter comments for that line, then select "Start a review". You can download the project code from GitHub if you'd like to use your favorite IDE to read through it (all of the projects will be readable to everyone in the class). You don't need to read the entire project (though you are welcome to if you wish); you only need to read the parts related to the area being reviewed.

Enter comments on GitHub, but do not submit your review yet. The comments will be saved and you will be able to see them, but no one else will see them until you submit the review. After we have discussed the relevant code in class, then you should submit your review. The "right" number of comments to enter is probably in the range of 10-20, depending on the complexity of the code you are reviewing and the number of useful issues you can identify (but don't invent issues if you can't find 10 meaningful things to comment on).

If you're not sure what to think about while reviewing code, focus on red flags: aspects of the design that suggest problems. Use the list of red flags in APOSD for starters, but feel free to add your own as well. Documentation problems are particularly easy to spot (and they are also common); APIs that are unnecessarily specialized are also good things to look for. Once you have found red flags, see if you can identify design changes that would eliminate them.

In addition to red flags, here are some overall questions to consider as you read code:

• How easy was it for you to find and understand the relevant code? Are the interfaces and algorithms clear? Does the documentation provide all the information you would need if you were going to make modifications to the existing code? Ideally there should be nothing that you find confusing. It may be easier for you to understand this code because you have already implemented something similar, so try to put yourself in the position of someone who doesn't already have experience with this particular application.
• How effective is the class decomposition? Do the classes have simple interfaces that hide information effectively? Are the APIs general-purpose? Is it obvious what features belong in (and out) of each class? Can you think of alternate class decompositions that would be better (e.g. are the classes too shallow? too deep?)
• Is the code simple and obvious? Identify the most complicated parts.

Review Presentations

Two of the reviewers for each project will present their code reviews in class, after the team has introduced its design. You will have 5 minutes for your presentation. This will not be enough time for you to discuss your review in detail, so pick out the most interesting and important issues. Use slides to structure your presentation and display examples. Start by saying what you liked best about the design, and what you think is the biggest opportunity for improvement. Then describe a few of the most important red flags that you found; be specific, and show examples. If you identified solutions to problems, pick the one that is most interesting and tell us about it. Your review must include one of the two following things:

• Find an example of bad documentation. Show the existing documentation (and its related code) on a slide, then show another slide in which you have rewritten the documentation to improve it.
• Or, alternatively, find an example of a poorly named variable. Show the variable (and related code, if any) on a slide, describe why this is not a good variable name, and propose a better name.

Review Topics

There is not enough time to review every aspect of every project, so each code review will focus on one particular aspect of the Raft design. Please focus your presentation or review on that topic. Here are some topic-specific issues for presenters and reviewers to consider; feel free to add additional issues.

RPC Communication

This topic will address the infrastructure for communicating between machines (both server-server and client-server), including connection setup, sending and receiving RPCs, and serializing request and response messages.

Presenters: What are the classes used for network communication? How do they work together? How does your mechanism allow requests to be issued concurrently to different servers? Is there a single mechanism for all communication, or is client-server communication handled differently from server-server communication? How do you serialize and deserialize the arguments and results for requests? What would it take to add a new request to the system?

Reviewers: Is this communication mechanism general enough to use for other purposes (either additional requests within a Raft server, or for applications other than Raft)? How cleanly is the communication mechanism separated from the Raft state machine?

Raft Server State Machine

This topic will address the Raft internal state machine (the code that implements Figure 2 in the paper, as opposed to the underlying communication mechanism). "State machine" refers here to the Raft consensus mechanism, not the application state machine where shell commands are executed after replication. This will include most of the code above the communication layer.

Presenters: What is the general organization of the Raft state machine? How does the state machine handle logically concurrent activities, such as requests to other servers and incoming requests? How are timeouts handled? Are there synchronization issues? If so, how are they handled?

Reviewers: to what degree is information encapsulated in the state machine, so that pieces can be understood separately? Is the structure of the state machine easy to understand? Is it easy to convince yourself that there are no races?

Persistence and Exceptions

This topic will address two issues: the mechanism for making information such as the current term and last vote persistent (and reading it back during restarts), plus the various mechanisms you use to handle exceptions. Exceptions related to communication are most important (both in client-server and server-server communication), but there may also be other exceptions that are important, such as those related to the persistence mechanism.

Presenters: What classes, if any, do you have that are dedicated to persistence and exceptions? How is information such as the current term and last vote made persistent? Is the mechanism general-purpose enough that it could be used for other purposes? How are exceptions handled, such as the crash of a peer server or a temporary malfunction of the network? Pick a particularly tricky exception or two, and walk through how they are handled. Can you make a convincing argument that you have handled every exception in the right way?

Reviewers: How well encapsulated is the persistence mechanism? Suppose another piece of data needed to be made persistent: how much would have to be changed to do that? How much complexity does the exception handling mechanism add? Is the mechanism complete (does it do everything it should)? Can you see ways to reduce the number of places where exceptions must be handled? Is it easy to convince yourself that the system will behave properly under all foreseeable failure modes? Consider exceptions in both client-server and server-server communication.

Schedule

Reviewers will present in the order listed above. Everyone should bring their laptop to class for the code reviews, both for your use in presenting and also so that you can browse the code online while we are discussing it.