 Okay. Yeah. Thanks everyone for coming along so late in the day. I know how tiring these days are, so I appreciate it. I've been looking forward to doing this talk for a while. Code Review is something that I think about quite a lot, and I feel like it's something that we should discuss more. I spend a lot of time doing Code Review, but yet relative to how much time I spend doing Code Review, I find that people are talking about it relatively little. So I think there's definitely ways that we can improve it and work on the process and so on. I've been, so just quickly to sort of tell you about myself a little bit, I've been working on OpenStack for about three years, and in that time I've been primarily working on triple O, and more recently I've been getting involved in the Mistro, the OpenStack workflow service. So really what I want to do is kind of reflect on my time in OpenStack on the review process, talk about what I think works, what doesn't work very well, and just share some of the lessons and things I've learned along the way. So this kind of makes a talk somewhere, sort of a beginner, but maybe slightly beyond beginner as well in some ways. Yeah, so as I was saying, with the amount of time I spend doing Code Review, I feel like just the small improvements we can make in the process, or just making people enjoy in general, it's quite interesting. Who here is actually involved in the OpenStack, sort of Code Reviews, who either as submitting changes or reviewing changes, just raise your hand. So that's most people. Now keep your hand up if you enjoy Code Reviews. Yeah, there's a lot of sort of hand waving, not many hands stayed up there at all, and that's the question that really interests me about Code Review, is I generally find it a very rewarding process, and I think that we can get a lot more out of it. But I also hear a lot of people complaining about it, and sure it's tough, I mean nothing's going to be perfect, but yeah, let's see what we can do to improve that and start talking about it more. But quickly to just reflect on why we're doing Code Review. I mean, when you ask people, sorry, if you look at the literature, it's quite interesting. So this is a book from a book called Co-Complete by Steve McConnell, and he found that, or he wrote, sorry, that the average effectiveness of design and code inspections was 55 and 60%, whereas unit test was only 25% for finding defects. Function testing was only 35%, and integration test was 45%. So I mean, it's really quite remarkable that his findings, it was based on the code review was the best thing at finding defects. And then this other paper, which is measuring defect potentials and defect removal efficiency, they found the code detection was up, it was 85, sorry, up to 85% for defect removal by average around 65%. Again, these are kind of remarkable numbers. So you can see why we're doing code review. But one of the problems I have with literature and these two quotes is that there's a lot of it's focused on bugs and defect removal. Whereas actually there's a lot more to get out of code review, but they're much harder to measure. A lot of the research uses defect counts because that's something finite, you can count it. But if you're looking at the other advantages, they're much harder to quantify. So this kind of brings on the question of the goals behind code review, what people are thinking when they're going into code review, what they're trying to get out of the process when they're doing it. Some people probably feel like they're just trying to merge code, some people, because they just want to get this contribution in other people feel like they're obligated to review code. And there's quite a lot of different goals. But when people ask, when people are asked why they do code review, the most common response is that they are looking to find bugs. So there's a paper called Expectations, Outcomes and Challenges of Modern Code Review. And they stated that while finding defects remains the main motivation for reviews, reviews are less about finding defects than expected and instead provide additional benefits such as knowledge transfer, increased team awareness and creation of alternative solutions to problems. And so the way that they've done this research is they actually worked with a couple of development teams in Microsoft. And they, initially they surveyed and questioned a bunch of the developers and they were asking them why they'd incurred review. And that's how they found the main motivation was to find bugs. But then they took a large number of code reviews. I can't remember the number off the top of my head. And they manually classified them to figure out what was the actual outcome of the reviews. And they found out that this was quite a big disconnect between why people felt like they were doing code review as to compared with what they were actually getting out of code review in the result. And so if you're actually curious in the breakdown of what they found, unfortunately the paper only specifically quoted two of the numbers, they showed the order. So I only have two of the actual percentages. But they were finding that code improvements within 25, sorry, 29% of the reviews. Then it was understanding. Sorry, sorry, I should explain. Code improvements is a change goes in for review. And then it gets some improvement to make it better. But it was originally what affectionately was working fine. So it's just the code view maybe made it more performant or more elegant or something like that. But actually the code initially was good. Then understanding. So this is to do with shared team understanding what changes are happening, what's happening in the project, social communications within the team. And then defects was down at only 14%, which is really quite low compared to the others. So you can see it's really trails off in the curve. So given that most people feel like they're going into code review to find defects, that's only actually happening around 14% of the time, at least in this particular case. So with these goals in mind, there are effectively two primary roles in code review. You have the author of a change and the reviewer of a change. You should ideally be doing both. And I definitely encourage everyone to do both, even if you don't necessarily feel qualified, but we'll come back to that one. But one of the issues I find in code review, or I think I've seen it sometimes anyway, at least, is it's almost framed as you're working against each other. So people are just trying to merge code and other people are trying to stop code coming in. I don't know, it's almost like you start as a conflict as an initial thing. And I think that's, I can be a bit of a sign of a toxic sort of culture. I don't think this actually happens too much in OpenStack. It's something I've seen more elsewhere, but it's quite an interesting thing to be aware of. And one of the reasons, this kind of leads me to one of the reasons why I don't like the name code review. I don't realistically think we'll ever change it at this point. It's something that's been used for far too long now. But at least in my head, I like to think of it more of a code discussion, a code collaboration, something where you're working together. And I'd be happy to take other suggestions for names because that's the best I've got. But really, it's all about thinking about the authors and reviewers working together. I mean, code review is just a collaboration tool. You should all be working with roughly the same goals of coming up with the best code and the best outcome, the best solutions to the problems. So while I said that you should definitely be doing both reviewing and authoring changes, there are still distinct roles. When you're thinking of an individual patch, you are either the author or the reviewer of that patch. You don't often change roles. So what I'd like to do now is just go kind of through the process, thinking about it from each point of view, and just share some sort of suggestions, tips, and ideas, that kind of thing. So we'll start first with authoring changes. And this is about how we can make, prepare your changes that you're going to send up for review to have a, like, a better chance of being merged, essentially. I mean, just basically something to grease the wheels and make the process go a bit more smoothly and reduce some of the back and forth and sort of the trip ups that happen in a lot of reviews. So the first one is maybe a bit of an oxymoron. But it's really important to make sure there's some kind of shared understanding and agreement about what you're doing. Depending on the size and scale of this piece of work, it could just be a brief discussion on ISE, or maybe you're working on a bug, or if it's something larger, you might want to work on a spec or a blueprint and make sure there's a decent amount of discussion beforehand. This is really about avoiding wasted effort. You don't want to invest a lot of time into writing code to then find out that it doesn't fit with the project for some reason. So make sure there's at least some kind of shared understanding there. Although having said that, I mean, you have to judge this yourself because sometimes things are very trivial. So you just want to do the code and then there's not much time wasted. Or there's occasionally times where the code speaks best and you prefer to work that way. You just have to be prepared that it doesn't necessarily mean this code will land. You might have to revisit all of it at some point. It's really important that you adhere to the project guidelines. And this is something that we do, I think, really quite well in OpenStack. The expectation of having tests is great across all projects, as far as I'm aware. The expectation of having documentation is not so great, but it's probably better than your average OpenSource project. But really, it's about making sure that you fit in. So while we might be one overall OpenStack community, there is still subtle differences between the different projects and how they work. Maybe some projects have a stronger focus on integration tests, where others do unit tests. Just make sure you're fitting in. Some do both and you need to make sure you're sort of satisfying all the requirements that are expected. And this is something that you can learn how to do by just asking what they expect from you. Or you can learn this by reviewing other changes. See what other feedback is going to other people. And this is partly why it's important to try and make sure you're doing both sides of the process. It's really important to provide context. And this slide intentionally doesn't provide context to make a point. So the review encoder is really quite difficult. So you've got to make it much easier for people. Oh, sorry. Make it as easy as possible. You can for people. Ways to do this is provide good commit messages. Make sure that you're explaining what you're doing and why you're doing it. There's quite a subtle difference there. And it's quite common that you'll see a commit message which only describes one of them. So what you're doing is adding a new function, for example. But then why are you doing that as well? It's like what was the actual end result? What are you trying to do? So it's good to explain both of these things. Why are you doing it as a new function? And why and what is the actual feature that you're exposing with this or creating with this? And of course, make sure that in your commit messages that you reference blueprints and bugs. This is something that we actually document very well upstream. I probably should have linked this here. But if you just Google for OpenStack commit messages, you'll find a very detailed document which explains all the expectations of commit messages and also will show some good and bad examples, that kind of thing. It's very useful. When you are putting a change up for a review, you want it to be small and contained. So when I was researching for this talk, I seen this tweet and I think it was posted as a joke really. But I think there's a really valid point behind it. And so they're saying that 10 lines of code in your patch and you'll see about nine issues, 500 lines of code and the reviewer will just be like, oh, that's fine. Some people might look at this and think, okay, I'll just submit bigger patches, they'll get merged more quickly. But that's the wrong way to look at it. The reason I think this happens is when you submit a small patch, your reviewer is going to be able to easily get their mind around it and really understand what's going on. So then they'll spot all the subtle problems that are there. But if it's a bigger change, then they'll maybe get to a point where they have an understanding of where everything connects to in their head but they just can't quite get the focus in on everything in the same way. So to back this up, there was a paper which is called Investigating Code Review Quality, Do People and Participation Matter. And it's no surprise, I don't think this is probably what I expected and a lot of people expected, but they basically found that larger patches lead to a higher likelihood of reviewers missing bugs. I think this is commonly stated by people but it's just nice to see this backed up with some research, which seemed pretty good. They used, for this one they used the Mozilla bug tracker and I can't remember which project it was for now but they basically, they compared the data with Mozilla's bug tracker and code review tools. One thing that I would say that we, I think we have a bit of a challenge with an open stack is Gerrit makes this harder sometimes. It's fairly easy to submit a sort of a series of patches that you're connected. But from a reviewer's point of view it's actually quite hard to understand that that patch is part of a series of commits. I think, because sometimes I'll be reviewing something and I won't necessarily notice that it's part of a chain and then you miss the context of it being part of a chain, which is really important sometimes. So I think Gerrit could do with a better way of exposing that to users or just representing it to people. And it's also quite a pain to update them as well until you get used to this of the get rebasing workflow. When you actually get your change, sorry, not finished, when you submit your change, that's when you should really think about that as being the start of the conversation. I think a lot of people will maybe work on something for say a couple of weeks, then they'll submit it and they, at that point they kind of feel like they are done by I think you should be more thinking about that you presenting it for feedback, that you asking for feedback at that point. And you can get around that long delay by just putting it up earlier for a review, maybe marking it as work in progress and then you'll probably find that some interested people will take a look at it as well. It's really just about opening up the conversation, opening it up to fill collaboration and feedback. One handy thing that I find is when I first open a review, I like to close my editor down whatever editor you're using. And then I'll go to Garrett and I'll quickly actually scan through and sort of review my own change. I'm not quite sure what it is, but I think it's possibly the mental switch from going from my editor to being Garrett puts me into review mode and I suddenly notice that I've done all kinds of silly things. So I'm quite often the first one to give myself negative feedback. When you've got your change up for review, it's really important to give up ownership. So again, when I was researching this, I was asking people how long they were spending doing code review just out of interest to find out some numbers. And this person replied they spend zero percent doing code review because coders act like they've painted a masterpiece and tend to debate every piece of feedback. So this this tweet didn't really give me enough information to know what's happening in their case. It may be maybe the feedback was too harsh and too severe. So people got defensive. Or maybe it was the the the sorry, the author of the change was just really defensive because they felt sort of proud of this work and they they weren't really ready to give up the ownership. So it's just really important that you can think objectively about your own changes when you put them up. It's quite difficult to do and I think this one maybe just takes time to sort of develop that feeling for the code. But the one one way I like to think about it is if you've ever looked at code that you've written maybe six months or a year ago, you tend to cringe at it. You don't really feel very happy about it. Try and think about the code that you're writing today like you will in a year. You'll be happy to then see it improved at that point. But really the the thing about reviewing code is it's actually it's quite difficult. It's quite a hard process. So when you're submitting your change for review, you should be really working to try and make it as easy as possible for the reviewer just helping them out as much as you can. So you're helping them to then help you back in return with their feedback. Then from the sort of the reviewer's point of view, these people are almost like the the project gatekeepers. They are they are the ones that are letting code in. So they've got this sort of responsibility to make sure that the code being merged is is at the right sort of level of what is expected for that project. And responsibility is really quite an important one. There should be a shared responsibility, I think, for every change for the between the author and all the reviewers. And this is why I feel like tools like get blame are fundamentally broken. They don't provide you enough context. So if you look at any patch that's been merged in OpenStack and you do a get blame on it. Sorry, not any patch. If you look at any file in OpenStack and you do a get blame on it, it will most likely tell you the author of that file. Sorry, of that of those lines, but it will not show you the people that reviewed and merged this code. So really, I just don't think that's very useful. I mean, it could be quite interesting to find to have something which pulled all that data together. And then I could say, OK, who are all the people that know about this file? And then it'll bring in the the commit, sorry, the get authors as well as the the Garrett reviewers. I also generally while on this topic have objections with the name for get blame because I just don't think it's a very nice command name. It's not necessarily be about blaming people. After all, you might just be trying to figure out where something came from. There's a quote that I like and I don't know where it came from. So I can't I can't reference the original, but it goes something like this. Code contributions are like puppies. Everyone thinks they're cute and everyone likes them unless you're a cat person, then just replace this replaced with cats. But you do need to walk them, feed them and look after them for the rest of our lives. So it's quite a big responsibility. And you can think about code in the same way. When somebody submits code for you and you merge it in, you then need to maintain it, test it and document it until you can deprecate it. And often times it takes quite a long time for you to actually be able to deprecate the code. So sorry, when I gave this talk before, I actually done a more generic version that wasn't sort of related to OpenStack. And I think that this one perhaps relates more outside of OpenStack just with the nature of the community and how active, how many active people we have in a different projects. But it's definitely important for maintainers to be able to say no to things that don't fit in the project or that perhaps have a maintenance burden which can't be met by the current core team. Something like that. So I touched on this before and really everybody should be reviewing, I think. So this is cause, non-cause, junior as developers, senior developers. There's a paper which I I forgot to reference on this slide. But they actually found out the quickest way for people to become better reviewers was by doing lots of reviews. So if you if you feel like you're not qualified to do reviews, then the only way to become qualified is by reviewing and helping out. And it's also a really great way to learn a project. So I also mentioned at the beginning that while I've primarily worked on Triple O, I've also been trying to help out in Mistral a bit in the last year, this year or so. And to start with what I'd done is I just started reviewing things on Garrett. Initially, I didn't really comment or even give a vote. I just kind of looked through things trying to see if I could understand what other people were saying, why the feedback made sense. And it was tough like the first of 10 reviews or so I went through. I just felt like I had no idea what was going on. But slowly, I started to get an understanding of what was expected, what was going on in the project. I fortunately I had enough time that I could I could invest this time into doing these reviews. But then really after I'd done that for a while and then I got to the point of trying to help out by fixing some bugs. It was so much easier because I just had this rough understanding of where things were. Like people were doing similar changes and then I seen, OK, they updated here, here, this test and this test. And it just gives you that extra context which is really useful. So yeah, so reviewing is really a great way to I think just to on board yourself or level up in the project and just know more about it. Automation is really key and this is something that again we do really excellently I think in OpenStack. We automate code style, test runs and a whole sort of suite of tests depending on the project. And this is really important. The code style one in particular is important because what you'll find is if you get feedback from Jenkins saying minus one, you missed this little pep eight thing, you will perhaps be annoyed but you'll understand it's an automated process. It's done without judgment. It's not not a problem. But if you get a negative review from a person and they're sort of picking at the style that you've used in the code, but really it doesn't matter that much. It can be I don't know, quite disheartening. It's not a very nice process, not a nice thing to experience. And unfortunately, you do see this in some projects and OpenStack. You will find that they have sort of specific styles that they like or things that they want to enforce, which are not automatically checked. And I think that is really a waste of sort of time and resources. So if we can't automate it, we should be moving on and just letting those changes get in because eventually you will miss some of these code styles that you don't quite like. So you're going to end up a bit inconsistent anyway. So it's really that important we should be automating it and that should be possible. It's really important that you're you don't try and review too much in one go. So this is another paper that I read and they basically found that around 200 lines of code an hour was a sort of a way you should cut you off your reviews. Another paper I read said 400 lines. So it's kind of a variable. But maybe if you take that as a 200 as a low ball, 400 is enough, look a higher, you should not really go over that amount in an hour. It's also hard because I don't know what language they were using with their with their research. 200 lines of Python is very different from 200 lines of Java, for example. But the real sort of takeaway is to try not to review too much. Make sure you kind of got you've got a fresh mind. Otherwise, if you go into reviews, you can you'll end up doing not a great job really. If you try and do one too many, you're like, I'll just do another one. Try and make some more progress. And I think the number is quite interesting because I think if somebody asked me to guess how many lines of code I could review an hour, I would have probably went quite a bit higher. But that's maybe just me overestimating myself. It's really important in your reviews to make sure you're giving constructive criticism and also giving people praise. It's really important. Sorry, it's really easy to point out only the bad things in your reviews, but really try and give people sort of encouraging feedback as well. So sometimes maybe somebody will have used a new feature, not a new feature necessarily, but a feature you didn't know about in Python or something that they're doing something with a library that you didn't know was possible or that maybe they're even using a library you've never seen before. Something like that. Just sort of let them know like, hey, I didn't know I could do that. That's cool. And then now you because you've learned something. So you've actually gained something really quite nice from this process. So it's good to let them know that you've gained that. And it's just it's really nice for everyone to feel that sort of hear that feedback. Interestingly, when I was practicing this talk a few months ago, I noticed an email notification pop up and somebody had actually commented on one of my reviews saying I learned something today. I had no idea that a doc string was sufficient content for a Python class. So in a Python class, you can have just a doc string. You don't need a little pass statement. I felt quite good that I'd helped somebody learn that and it kind of helped back at my point. So I was like, that was great. That was one of the better emails that day. Again, it's this is more this sort of comes down to language, which can be quite a subtle and difficult thing, especially when we've got such a wide reaching community. And I guess a lot of people are not speaking or not writing in the first language. So it can be a bit difficult. So I'm definitely not blaming anyone or accusing anyone in this case. But I think it's just something to be aware of. Think about how you're writing things down and how that will come across to the other person. So this is an example that I'd seen in another talk. And they're basically saying so if I was sitting next to somebody and I said, oh, hey, why didn't you do that? That sounds quite nice. It sounds quite casual. It's not too bad. But when it's written down, it's why didn't you? Sorry, I can't sound mean. I'm not very good at that. But it can come across as quite a negative way of writing it. So you could maybe replace it with something like, could we do this? And I don't think it's a great example. But just really think about how what you're writing comes across to the person. It's also really important to make sure that you're asking somebody rather than telling someone to do something. So maybe you're reviewing a patch. You see something and your immediate reaction is, oh, no, you should not do that. You should be doing it this way. They might have already tried that way and you've missed something. And they then chose this other way for a good reason. So rather than saying, you should have done it this way, you could say, hey, did you try this? But again, this kind of comes back to the point I was making before about reviewing code as hard. It's also really hard to write code, especially in a project like OpenStack. It's such a large number of projects covering such a wide range of different features and functionality that really we need to just make sure we're helping each other in each side of the process. So giving good feedback helps these reviews move along a lot better as well. And at the end of the day, I said this at the beginning. It's really the process is all about collaboration and working together, making sure that you're giving people enough context and descriptions so that they can understand what you're trying to do and give you a valuable review. And making sure that we're automating as much as possible, removing all the grunt work to bots and build jobs that can do these kind of things. And also making sure that you'll count yourself when you're spending your time reviewing, you don't push yourself too hard as well. And that pretty much brings me to the end. I think I went through that far too quickly. So hopefully everyone understood me. But please do get in touch if you're interested in this or come and speak to me after. It's something that I think is really an interesting subject. I've got a whole bunch of sort of side project ideas I'd like to do something like analyzing the, sorry, for example, we have a whole lot of data in our OpenStack Garrett. I can't remember how many hundreds of thousands of reviews we're out now. But it seems to me like there must be something that we can learn from that, try and create some kind of feedback loop and see how the community is doing, what kind of, how everything's working overall. There are some projects which look at these things a bit like Stack Analytics. But I'm thinking more specifically just to the code review process. But otherwise, thank you. Are you taking questions? Yes, yes, sorry. I'm having to take questions. Great. It's fairly common to take the conversation that's happening on a review into IRC for a little while and sort of figure things out and kind of smooth things over. Once that's happened, do you have a preferred thing that should happen back on the review? Should it link back to the IRC log? Should there be some summary of that conversation or do you just kind of not worry about it? So I tend to, in that case, copy a summary or perhaps even just the actual transcript from IRC into the review. The reason being that you want to provide that context for following on reviewers so that they can see it there. And I hadn't actually thought about linking back to the IRC logs, but that could work as well. So I don't have a strong preference as long as the two are linked. Yeah, so just as long as it's not last. Yeah. So the second question is in other environments, code review is often sort of a process of essentially saying I can live with it. Whereas I think in OpenStack there's tend to be a stronger bias towards we want this to be closer to perfect before it ever merges. Is that, first of all, would you agree with that assessment? And secondly, what forces do you think are involved in that happening? So I do agree with the assessment. I think that's something which probably happens in most open source projects. I think the reason we need to have something essentially be production ready for it to be merged is that you don't really know when a contributor is going to disappear. And that's really important. If you're thinking of a closed environment where everybody is working for the company, you know that most of the time people are not going to just disappear. You get a notice when people are going to quit that kind of thing. So unless there's something horrific happens, you'll always have people hanging around to follow up on this work. But in open source and in OpenStack, sometimes people will submit changes and then they will never be back. So yeah, I think that's the main reason. Is it possible somehow to replace code review with peer programming? I don't know whether anybody does it in open source, but the same things happened. I mean, collaboration, sharing knowledge and finding defects and et cetera. Is it possible to do it in open source, synchronize and time zones, find time with somebody? Yeah, so I think it is possible. And certainly in some environments, I do know of people doing that. I don't know of anyone doing that perfectly in open source. However, in triple O, I have peer programmed on people with remote terminal sessions. And basically one of us ends up submitting the review and then the other will be the first review. I think it's good that we then have a second reviewer, which is somebody that's not been part of that peer programming process. So just to respond to that, in NOVA, we've discussed the idea of actually doing peer programming on opposite time zones. So sort of asynchronous peer programming across the same review. So you have two people who are in opposing time zones or slightly overlapping times where one person owns the patch during that 12 hours and somebody else owns it in another 12 hours. And so that you're pairing across the actual review. And so the idea is that the two people are responsible for the same patch. And so they co-author it, but asynchronously. Well, each time as the day moves forward while you're working on the thing, you review the stuff that the other person's done as you integrate it into your understanding of what's going on and then also integrate the reviews from other people who are just reviewing. I don't think it's been, we haven't actually, as far as I know it hasn't actually actively been done yet, but it's something that's been talked about because there's been problems with people taking on too large a load and that if you can distribute the load, one of the ways to do that is to basically co-chair in a way. I've seen a similar thing happening like that, but it tends to be that you still end up with two formal reviews happening afterwards as well. I think that might just be because a lot of the processes in OpenStack haven't really taken that into account. So people aren't quite sure how to handle it. But yeah, it would be interesting to see how it works out for Nova. OK, I think that's me then. So thank you all very much.