I wrapped a 2-year stint at Facebook recently and my experience was similar to yours.
Some tools were amazing and cool and I really appreciated how they evolved over time to manage huge amounts of resources. But most were subpar when compared to OSS tooling - outdated, deprecated functionality, very little documentation, very few people working on them. The common approach was to learn about "the duct tape" way to make something work, then pass it on to new engineers.
An example would be tool X for working with diffs (PRs). It's the latest and greatest, except that it only covers 75% of its predecessor's, tool Y's, functionality, so you end up learning both. Tool Y has been "deprecated" for the past 3-4 years. Some of its features don't work, but you'll only know when you try and execute them.
The thing that bugs me most about my company’s tools are the ones that are enough younger than OSS solutions that someone either didn’t look very hard, or didn’t want to find anything (so they could write their own). Other priorities come along and those tools eventually can’t keep up with OSS alternatives, but the apologists take over.
You will not continue to get accolades for tools you wrote three years ago. The only “value” you derive is the time and effort it saves, offset my the effort expended. The time and effort expense of external tools are often lower. And, when the tools are annoying, you can commiserate with your coworkers instead of being the target of their criticisms. Which is often under-appreciated.
This was actually pretty useful in OpenStack back in the day. The reviewers who couldn’t yet approve code could only put +1s on there (and there were a lot of low quality reviewers who slapped these everywhere) so it was very obvious when a patch still needed attention from a commit-privileged dev.
Then -1 was standard review feedback of stuff that needed improving and -2 would come from commit-privileged devs when the patch fundamentally didn’t fit with the project direction.
1 = WTF?
2 = Needs Improvement
3 = Good Enough
4 = Better than Good Enough
5 = Almost Perfect
Daniel Kahneman recommends a scale like this for evaluating job candidates across core competencies. It's a surprisingly powerful little heuristic.
-2 = WTF
-1 = Needs Improvement
0 = Good enough
1 = Better than Good Enough
2 = Almost Perfect
It's even intuitive, since anything above zero is 'above and beyond' and anything below zero isn't good enough yet.
-1: Changes Requested
-2: Declined
1: Approved
2: Accepted
Communication is hard. People actively in the team probably have the context to understand the differences, but that implied context is what tribal knowledge is made of.
OP indicated that there were non-trivial "non-linear" differences going from 1 to 2 and -1 to -2. The use of numbers implies a relationship that doesn't really exist. The use of accurate language makes the actual meaning immediately obvious without the need for implied context. For example, I had no idea that only privileged devs could give 2s. "Accepted" and "Declined" do imply a finality that more accurately mirrors the intended usage.
This may seem overly analytical for something that is easy to just explain, but this kind of thing build up in layers.
Gerrit, Reviewable and Phabricator are very similar, except that Critique is much more polished.
Having used all three, Gerrit works best and the new UI is starting to look pretty good. Reviewable has great UX, but inherits some of the disadvantages of GitHub PRs.
[1]: https://books.google.de/books/about/Software_Engineering_at_...
I think they kept the same keystrokes, though.
That is one nice thing about Critique is although it's a webapp you can drive it pretty much entirely by keyboard. It has the feel of an old usenet newsreader like nn, in a way.
But yeah... even I find the names difficult to handle and remember, even after using Phabricator for more than 4 years now. Nevertheless, it is a great piece of software, especially the code review process is top-notch.
More pixels != to more information. The visuals and varieties in ways to present data in grafana always seemed more intuitive for me, being able to lay stuff out in different ways was great. I worked on a G-on-G project at Google and used both grafana and viceroy. I never had an issue with grafana on mobile, but I was never was able to see viceroy on a mobile browser because of the Corp network.
I'm sure viceroy is more performant, and I'm not denying that, I may also be scarred a bit because I found writing mash queries an arcane nightmare, and those are related to viceroy.
The default (rpc latencies and etc) views and metrics you get are amazing however.
Mash is indeed arcane, although gmon (viceroy) makes it worse than usual.
It's still a big step up from, say, GitHub PRs, and the UX has improved significantly.
Exactly my feelings about Phabricator. GitLab simply runs circles around it if you consider the issue tracking, CI etc. as part of the problem.
This sounded awesome to me (imagining some Fortran code behind it), but disappointingly the cited screen shot only has the range [-1,0,1] which is hardly the same.
No, I'll take Github PRs any day. They could be better, of course.
Why not just leave them uncollapsed and allow one to mark them as resolved when the change is actually resolved. The other problem is that it's difficult to see what actually changed when a line is marked out of date without having to scan through the entire updated diff and then jump back and forth between the diff and conversation view.
Collapsing them by default makes it difficult to find the comment by scanning. If they were uncollapsed until I marked them as resolved, then I could at least use the find feature in the browser and search for my username to find my comments.
Except that if the overall diff line I choose to comment on about the commit message changes in some way, then the comment is collapsed, which makes it difficult to keep track of what needs to change.
> since by the time the PR is done the commit msgs are pretty much not changeable, except for the last one
They can be changed via a git rebase.
Link: https://github.com/ezyang/ghstack
If you're interested, I'm happy to talk you through it - just book some time here: https://calendly.com/ericyu3/15min
Then, we can run git rebase -i --autosquash --keep-empty to handle applying the changes requested to the correct commits at the end of the review process.
Maybe you'll build the framework & subfeature A together, because you need some bit of meat in there to properly figure things out and be able to test things
On Gerrit you'd probably create two commits which get pushed as two CLs
* CL1 Create Thing framework * CL2 Implement A for Thing
They're now pushed for people to review. While they're looking at them, you continue working on, making CL3 implementing B
* CL3 Implement B for Thing
One of your coworkers points out an issue in CL1, so you fix it (by amending the commit) and repush the stack. Now your stack is
* CL1 Create Thing framework [v2] * CL2 Implement A for Thing * CL3 Implement B for Thing
CL1 & CL2 get approved so you merge them (though typically with Gerrit they're cherry picked - CLs are individual Git commits). You push up an implementation of CL4, so your stack now looks like
* CL3 Implement B for Thing * CL3 Implement C for Thing
The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack). When you're working in Git you typically (unless for whatever reason you have multiple unrelated stacks on the go, which is relatively rare) just work off of the master branch, so all the commits between `origin/master` and `master` - i.e. the set that automatically show up in `git rebase -i` and similar tooling - are your stack. When you pull, you `git pull --rebase` (or rather set the config for your machine or repo to default to that). When you've revised something in your stack or want to add something to it, you just do "git push HEAD:origin/refs/for/master" update Gerrit with the latest version of your stack
It takes a little time to get used to (and you have to learn how to hold `git rebase -i` properly), but when you're used to it it's immensely more productive than the Github/clones PR flow (and doesn't involve manual branch juggling, etc). I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged)
When I'm in this situation when dealing with something which is PR based, I tend to end up merging CL1 & CL2 (because its easier to review things when you have an example) and hang on to CL3 & CL4 on my local machine until the first one gets merged
What prevents a situation like CL2 getting approved and merged prior to the resolution of CL1? Or more generally, what ensures that the ordering of commits in a set of CLs in a given stack is preserved prior to merging?
> I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged)
Rather than stacked PRs (which essentially doubles the number of commits since Github (and possibly Gitlab) introduces a merge commit for each PR even if it's a fast-forward merge), would it not be better to just combine CL1 through CL4 into a single feature branch where each commit corresponds to a CL? It would make for a large PR, but it can be reviewed on a per commit basis.
Perhaps the operative phrase being "Many years ago". I currently work at Google, previously I worked at Square. Of the two, I generally prefer the OSS and off-the-shelf tooling at Square. Some things really are better at Google (code search and Blaze are definitely a big improvement over what we had), but many of our monitoring and CI tools feel antiquated and cluttered with inscrutable debris. Or take Gerrit, where each change has 3 different IDs and the developers decided that every workflow status should be expressed as an integer in the range [-2, 2] for some reason [1].
They were probably amazing (and much simpler) tools back in the day, but the world has moved and we're somewhat constrained by what's familiar.
[1]: https://gerrit-review.googlesource.com/Documentation/images/...