Java Owners

Our goals:

  • Ensure that all java libraries/binaries use a unique version of a given third party library
  • Make adding/maintaining third party packages as painless as possible for everyone involved
  • Ensure all legal requirements are met

What we actually do

SLO: Respond to all new/ongoing reviews within a work day

The more responsive we are to submitters, the more willing they are to do stuff when we ask. Try to check for pending third party reviews when you come in, right before/after lunch, and before leaving. By checking three times a day, submitters feel that we are working with them vs. a barrier. The check usually shows nothing to be done.

If you will be on vacation take yourself off the rotation and hand off any pending reviews

gwsq, which assigns CLs, will automatically exclude reviewers who are OOO (using Critique's OOO detection). If you want to exclude yourself from reviews but don't want to add a calendar event send emailremoved@ a CL like this example.

Review CLs

1. Adding a new package or a new, unused version of an existing one

General Checks

  • If they chose a binary format, ask them to revise the CL to the preferred source-only format. If this is not an option, direct them to fill out the appropriate issuetracker exception template to be reviewed by ISE-TPS. Please wait for a resolution to be reached on the bug before approving the submission.
  • No more mocking frameworks -- we use Mockito. (If they push, refer them to emailremoved@)

Checking details of the CL

  • Ensure the CL matches appropriate template structures in third_party/java/SAMPLE_NEW_PACKAGE, etc.
  • Make extra sure the package_group and package statement are present. This ensures they have to upgrade everyone. NO EXCEPTIONS.
  • Check the contents of the jars
    • The jars should contain the code for that package and that package only, no embedded dependencies.
    • The source jar should be rooted at org/... or com/..., not src/org/...
      • GWT packages will have the source and binary code in the same jar, because the GWT compiler needs access to both to operate.
    • Check against go/packagemap to ensure the same packages are not already present in a different place.
    • If there is a legacy issue with that package embedded in another package, extract it, then (if necessary) upgrade.

If they are doing an upgrade and the package isn't to the new standard

  • The new CL should be to the new standard (including upgrades for deprecated binary-only formats - these OWNERS should move to source based builds as part of the upgrade maintenance work)

  • If the current rule name isn't to standard rename it google wide for them while they do CL#1.

    • //devtools/scripts/replace_string will create a CL fixing all occurrences in google3.

      `replace_string` `//third_party/java/proj:badrulename`
`//third_party/java/proj`

      Running the script with no args gives more details.

    • Check the CL by rebuilding the third party package, and running blaze buildlint on each affected file.

    • If there are more than 2-3 fixes, send the CL to a google/java/OWNER for approval.

  • Check google wide for any references to the version specific rule. If there are any, create a CL converting all the references to use the unversioned rule in the parent package. The instructions how are about five lines up.

2. Throwing the switch to use the new version

  • Ensure that the CL is as small as possible, usually only touching two BUILD files
  • Verify that either the impact is small, or that some reasonable verification was done (Presubmit, Submit Queues, etc)
  • Verify the access restrictions to the old version are removed to enable teams to do local emergency rollbacks

3. Deleting the old version

  • Do a go/cs for the old package to ensure no one is using it.

Additional duties for the Lead

There are three main additional areas of responsibility for the lead.

Handle Problems

Sometimes unusual cases or disagreements will crop up, and it's your job to make sure they're evaluated and adjucated fairly.

Make sure you read all mail that goes to emailremoved@ that isn't a code review, it's almost always either a reviewer asking what to do or a package owner asking a policy question. Problems also show up during reviews.

If someone is asking for something that's not a good idea, just say no as kindly as possible, being increasingly firm if they object. If they're balking at a third-party-removed rule, feel free to direct them to emailremoved@, who will tell them no.

When people find themselves in an unusual situation, you have to use your judgment to decide whether to make an exception for them; some get granted and others get refused. Some guidelines to keep in mind when deciding:

  • Our responsibility is global
    • Our primary responsibliity is to ensure that google3 as a whole works properly, now and in the future. Other things are secondary.
    • Just because something is inconvenient for an individual team doesn't mean they can ignore the rules. The rules are there for a reason.
    • At the same time, the rules aren't valuable on their own, they're valuable because they prevent problems. Sometimes teams want an exception from the rules because they can avoid the problems another way, and that can be a good compromise. Also, sometimes a team's situation will show you that the current rules aren't the right ones.
  • Incremental progress is okay
    • If a package is in bad shape, sometimes the best thing is to fix the most egregious problems and leave smaller ones for the next update.
    • Don't let anything dangerous linger (indeed, email owners proactively if you discover it), but formatting problems and the like can be put off if the team is fixing more important things and you want to preserve good will.
  • Not all packages are equal
    • A giant, staffed, actively-maintained package like GMSCore gets more leeway than the typical package.
    • Some projects have shown that they're very responsible. //third_party/appengine had a complicated setup with a multiple version exception for many years, and they built build tooling to ensure that it never caused any problems. If they asked for some exception, they got it.

If things get particularly heated (it's rare, but it happens), we're the ultimate arbiters of what happens in //third_party/java, and you have a lot of powerful tools at your disposal to enforce that. If it's called for, you can remove people from OWNERS for their packages, delete packages, restrict their visibility, revert CLs, contact managers, or direct people to third-party-removed. Some of these are nuclear options that expend significant political capital, so be cautious, but they're there.

Maintain the Ecosystem

You need to put in some regular work to make sure that //third_party/java continues to run smoothly. The biggest component of this is adjusting policies when called for, whether because someone has found themselves in a situation that the rules handle poorly or because the world has changed and our policies need to change along with them.

Read the threads on emailremoved@, it's the best way to find out what's happening in third-party-removed land. They sometimes change policies or add new tools and don't tell us directly, so we have to find out from observation.

Keep in mind that //third_party/java has an unusual setup. You sometimes need to be proactive in pointing out places where the overall rules for //third_party don't work for our area and ensuring exceptions are made or policies are adjusted.

Maintain the Team

This work is fuzzier, but just as important as the other two. Make sure that the team feels like they're in a good place to do the work they need to do.

Ensure that everyone knows you have their back. Most reviews are routine, so when something weird comes up or a package owner decides the rules don't apply to them, reviewers can sometimes feel out to sea. Try to spot reviews that have a lot of messages on the review thread and spot check if the reviewer needs help. If it's not obvious if your input would improve the situation, I like to send a private message to the reviewer telling them that you're watching and if they need anything to just say so and you'll intervene.

Be the cavalry when it's called. Reviewers will ask you to intervene from time to time, and you need to make sure they're glad they asked. If the reviewer made a decision you disagree with, either talk with them privately and let them handle it in public or own the choice yourself publicly (e.g. " Reviewer is right that our rules say you have to do whatever. If you think those rules aren't right, let's chat about it.") Don't throw reviewers under the bus, even if they've made a mistake.

Make sure the workload is reasonable for the number of reviewers on the team. For the workload we had in the 2016-2018 timeframe, I liked having a minimum of 4 reviewers (including the lead). If you think you're in danger of having too much review load, email java-users and ask for volunteers. You can look at previous calls for volunteers for phrasing suggestions.

Training up new volunteers takes a while, so don't let it become an emergency. A recommended process for new volunteers:

  • Have the volunteers read: this page and go/thirdpartyjava.
  • Have the volunteers study the templates (..third_party/java/SAMPLE_NEW_PACKAGE, ..third_party/java/SAMPLE_JAVA_SRC_PACKAGE, ..third_party/java_src/SAMPLE_JAVA_SRC_PACKAGE)
  • Explicitly ask if they have any questions about either. It is amazing how often "I got it" becomes several questions.
  • Review every CL that they review until they consistently catch everything.
  • Respond on the CL with any issues they missed. This needs to be timely enough to avoid bad submissions.
  • Consider setting up an email filter for "to: emailremoved@, from:".