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.
To start off each code review, the presenting team will spend 10-15 minutes to address the following issues:
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).
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); information leakage is also a good thing 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:
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:
Each code review will focus on one particular aspect of the Raft design. Here are some topic-specific issues for presenters and reviewers to consider; feel free to add additional issues.
This topic will address the infrastructure for communicating between servers, including connection setup, sending and receiving RPCs, and serializing request and response messages.
Presenters: What are the classes used for communication between servers? How do they work together? How does your mechanism allow requests to be issued concurrently to different servers? How do you serialize and deserialize the arguments and results for requests? What would it take to add a new requests 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?
This topic will address the state machine (the code that implements Figure 2 in the paper, as opposed to the underlying communication mechanism). 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?
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.
| Date | Topic | Presenters (Repo) | Review&Present | Review |
| Fri., 1/31 | RPC Communication | Peter/Anirudh (raft5) | Jack, Mathew | Robbie, Ali |
| Xinyi/Changyu (raft3) | Nick, Max | Kate, Yuqi | ||
| Wanqi/Felix (raft2) | Wil, Yuqian | Anna, Leon | ||
| Mon., 2/3 | State machine | Jack/Anna (raft9) | Yuqi, Changyu | Wil, Peter |
| Max/Ali (raft6) | Robbie, Anirudh | Xinyi, Wanqi | ||
| Mathew/Leon (raft8) | Kate, Felix | Nick, Yuqian | ||
| Wed., 2/5 | Persistence & Exceptions | Kate/Wil (raft1) | Ali, Leon | Jack, Changyu |
| Yuqi/Yuqian (raft4) | Anna, Xinyi | Anirudh, Felix | ||
| Nick/Robbie (raft7) | Peter, Wanqi | Mathew, Max |
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.