I would like to see this fixed from the infra side. There's possibly a related issue where codereview won't show diffs between two different patchsets for histograms.xml with error "File too large.".

The single histograms.xml file is required because it's needed by UMA dashboards to have metadata about histograms. It's also used by presubmits to make sure there are no conflicts between histograms.

An alternative where we can have multiple files would require e.g. presubmits to process all the different files and also for backend infrastructure to be more complex, so I think it would be better to fix the codereview infrastructure.

This is not easy to fix at all, because these reverts are created on Rietveld, an AppEngine app, with strict memory limits. But we are working towards making Gerrit codereview an alternative for Chromium, and Gerrit can handle much larger reverts.

I think the file is just a few megabytes. Can you elaborate on the memory limits you are referring to?

I believe it's possible to have a higher memory class in AppEngine so I could imagine it could be as simple as upping that (and paying a bit more for it).

+rmistry@ for why limits were set in Rietveld

The root cause is that the base file does not get uploaded to Rietveld by depot_tools. We have run into this in the past, my comment from https://bugs.chromium.org/p/chromium/issues/detail?id=476418#c1 :
"""
Looks like this happened because of tools/metrics/histograms/histograms.xml being a very large file. The base file does not get uploaded and due to this Rietveld cannot compute the reverse diff.

A way of determining when a file is too large for Rietveld is that the side-by-side diff for that file does not work, Eg for histograms.xml:
https://codereview.chromium.org/1060993003/diff/220001/tools/metrics/histograms/histograms.xml
"""

Even if depot_tools allowed large files to be uploaded it is likely that Rietveld would not invert the patch. To revert, Rietveld loads all patchsets in memory and attempts to create an inverted patchset. It checks for how large the patchset is here: https://chromium.googlesource.com/infra/infra/+/master/appengine/chromium_rietveld/codereview/revert_patchset.py#113
Without checking for file sizes the app will run out of memory, crash, and restart. This happened multiple times in my one-click revert tests before the feature was launched. As comment #5 says we could probably up the memory limit to handle the current size of histograms.xml but that would not solve the problem for all large files and would not work if histograms.xml kept getting bigger.

I recommend leaving the current checks the way they are because this should not be an issue when we move to PolyGerrit:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Bgwj-HTCGdQ/KnoK8U0pAgAJ

+1 for rmistry@ conclusion. Thanks!

PolyGerrit sounds like something that is far away. (At least I've not heard of it being announced on chromium-dev@ so I assume it's in very early/planning stages.)

If that's true, and fixing this currently could just be a matter of increasing a constant in depot tools and upping the memory class in the appengine instance, I think it would definitely be worth it to do that in the short term.

For context, this affects me basically every day as histograms.xml reviewer, since I can't see histograms.xml diffs between patchsets. And limiting our ability to revert patches easily is also terrible.

Can we at least try the above changes if they are low hanging fruits like they sound?

actually polygerrit and normal gerrit for chromium project is not as far away as you may think :)[1] But we do need 1-2 months until it's possible to try polygerrit changes in CQ and land them.

Adding Andybons@ for considering Rietveld work.


[1] In fact, "git cl upload --gerrit" probably already works for you (but be sure to run it on a separate branch from your rietveld CL, just to avoid any confusion:
  # suppose you are on your "work" branch with Rietveld CL attached
  git checkout -b work-gerrit
  git cl upload --gerrit --bypass-hooks

"PolyGerrit sounds like something that is far away. (At least I've not heard of it being announced on chromium-dev@ so I assume it's in very early/planning stages.)"

I linked to the announcement on chromium-dev in comment #7:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/Bgwj-HTCGdQ/KnoK8U0pAgAJ
which says "The conservative estimate is Q4 of 2016".


In the 2.5 years since one-click revert for Rietveld has been launched this (inability to revert large files) has come up maybe 3-4 times. I would hesitate to increase the load on Rietveld because it is already fragile, but the decision is Andy's.

I am happy to review any patches provided they are simple in nature.

PolyGerrit is already being used and usage is increasing. Taking effort away to patch depot_tools/Rietveld (and the possibility of bugs from that patch) is something I‘m very wary about.

So, if it’s affecting your productivity then submitting a fix is your best bet until the switch.