Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/godoc/redirect: offer Gerrit/Rietveld CL disambiguation when needed #28836

Closed
bradfitz opened this issue Nov 16, 2018 · 21 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

Go's CL numbers as assigned by Gerrit have started to collide with the lower numbers in the sparse set returned by the old Rietveld code review system we used to use.

Our https://golang.org/cl/NNNN handler should render a disambiguation page when the CL exists in both.

/cc @bcmills @dmitshur @andybons @adg

@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Nov 16, 2018
@bradfitz bradfitz added this to the Unreleased milestone Nov 16, 2018
@gopherbot
Copy link

Change https://golang.org/cl/150057 mentions this issue: godoc/redirect: improve Rietveld CL heuristic

gopherbot pushed a commit to golang/tools that referenced this issue Nov 16, 2018
Go's CL numbers as assigned by Gerrit have started to collide with the
lower numbers in the sparse set of CL numbers as returned by our old
code review system (Rietveld).

The old heuristic no longer works now that Gerrit CL numbers have
reached 150000.

Instead, include a map of the low Rietveld CL numbers where we might
overlap and bump the threshold heuristic up.

Updates golang/go#28836

Change-Id: Ice719ab807ce3922b885a800ac873cdbf165a8f7
Reviewed-on: https://go-review.googlesource.com/c/150057
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@aclements
Copy link
Member

Is there any way we could knock out the Rietveld CL numbers from Gerrit's numbering space?

@bradfitz
Copy link
Contributor Author

@aclements, yeah, I had also mused about that this morning with Bryan.

@andybons, could you talk to the Gerrit team about that?

The initial numbers of concern are listed here:
https://go-review.googlesource.com/c/tools/+/150057/5/godoc/redirect/rietveld.go#36

To get numbers higher than 300,000, run:

$ cd $GOROOT
$ git log 7d7c6a9..94151eb | grep "^    https://golang.org/cl/" | perl -npe 's,^\s+https://golang.org/cl/(\d+).*$,$1,;' | sort -n | uniq

@andybons
Copy link
Member

@andybons, could you talk to the Gerrit team about that?

Will do.

@andybons
Copy link
Member

They can change the number to whatever we want in the future. Where do we want to start at now?

@bradfitz
Copy link
Contributor Author

The problem is the Rietveld numbers were sparse and got very large. We don't want a dozen digits in our CL numbers. That's more painful than the annoyance cost or occasional collisions.

I suppose we could look for the largest early gap.

@gopherbot
Copy link

Change https://golang.org/cl/150577 mentions this issue: [release-branch.go1.11] godoc/redirect: improve Rietveld CL heuristic

gopherbot pushed a commit to golang/tools that referenced this issue Nov 21, 2018
Go's CL numbers as assigned by Gerrit have started to collide with the
lower numbers in the sparse set of CL numbers as returned by our old
code review system (Rietveld).

The old heuristic no longer works now that Gerrit CL numbers have
reached 150000.

Instead, include a map of the low Rietveld CL numbers where we might
overlap and bump the threshold heuristic up.

Updates golang/go#28836

Change-Id: Ice719ab807ce3922b885a800ac873cdbf165a8f7
Reviewed-on: https://go-review.googlesource.com/c/150057
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
(cherry picked from commit e77c068)
Reviewed-on: https://go-review.googlesource.com/c/150577
@dmitshur
Copy link
Contributor

golang.org has been redeployed with the CL 150057 change applied, so short links like https://golang.org/cl/150057 are correctly pointing to Gerrit CLs now.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 26, 2018

I believe this issue is now fully resolved by CL 150057, redeploying golang.org, and @andybons taking measures to prevent future Gerrit CL numbers from overlapping with existing Rietveld CLs. We won't need a CL disambiguation page.

Closing to reflect reality (but feel free to re-open if I'm missing something, or there is more follow-up work to be done).

@bradfitz
Copy link
Contributor Author

bradfitz commented Nov 26, 2018

and @andybons taking measures to prevent future Gerrit CL numbers from overlapping with existing Rietveld CLs. We won't need a CL disambiguation page.

I'm on that email thread and I haven't gotten any update from the Gerrit team.

Duplicates are still possible. They'll be rare, but possible.

I'd like Gerrit to mark all our old Rietveld numbers in https://go-review.googlesource.com/c/tools/+/150057/5/godoc/redirect/rietveld.go#36 as reserved so they'd never be handed out to us again.

Reopening until we do one of these: a disambiguation page, or modifying Gerrit.

@bradfitz bradfitz reopened this Nov 26, 2018
@bcmills

This comment has been minimized.

@bradfitz

This comment has been minimized.

@bcmills
Copy link
Contributor

bcmills commented Nov 30, 2018

Looks like we already missed the window on avoiding collisions:
https://codereview.appspot.com/152078
https://go-review.googlesource.com/c/go/+/152078

@bradfitz
Copy link
Contributor Author

bradfitz commented Dec 3, 2018

@bcmills, we can still minimize them going forward if Gerrit folk can reserve those IDs for us.

@dmitshur dmitshur self-assigned this Jan 9, 2019
@dmitshur
Copy link
Contributor

dmitshur commented Jan 9, 2019

We just hit this again in #29633 for CL 157099, which exists both in Gerrit and Rietveld. There are 837 more numbers that follow, so we're going to continue hit the issue for a while. There haven't been any new updates on the Gerrit email thread.

Our https://golang.org/cl/NNNN handler should render a disambiguation page when the CL exists in both.

We can query the Gerrit API to figure out if a Rietveld CL is also a Gerrit CL (and cache results). I'll implement this and send a CL.

@gopherbot
Copy link

Change https://golang.org/cl/157197 mentions this issue: godoc/redirect: display Gerrit/Rietveld CL disambiguation page when needed

@dmitshur
Copy link
Contributor

dmitshur commented Jan 10, 2019

@gopherbot Please backport to Go 1.11, so this change can be deployed to golang.org.

@gopherbot
Copy link

Backport issue(s) opened: #29645 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@dmitshur
Copy link
Contributor

Reopening for deploy of golang.org with the fix applied.

@dmitshur dmitshur reopened this Jan 10, 2019
@dmitshur
Copy link
Contributor

golang.org has been redeployed with the fix.

For example, https://golang.org/cl/157099 now displays a disambiguation page rather than immediately redirecting to Rietveld CL 157099, since a CL with that number exists in both systems.

@adg
Copy link
Contributor

adg commented Jan 10, 2019

Nicely done.

@golang golang locked and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants