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

Comments

Projects
None yet
7 participants
@bradfitz
Copy link
Member

bradfitz commented Nov 16, 2018

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 this to the Unreleased milestone Nov 16, 2018

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 16, 2018

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

godoc/redirect: improve Rietveld CL heuristic
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

This comment has been minimized.

Copy link
Member

aclements commented Nov 16, 2018

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

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Nov 16, 2018

@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

This comment has been minimized.

Copy link
Member

andybons commented Nov 18, 2018

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

Will do.

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Nov 19, 2018

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

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Nov 19, 2018

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

This comment has been minimized.

Copy link

gopherbot commented Nov 20, 2018

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

[release-branch.go1.11] godoc/redirect: improve Rietveld CL heuristic
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

This comment has been minimized.

Copy link
Member

dmitshur commented Nov 21, 2018

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

This comment has been minimized.

Copy link
Member

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).

@dmitshur dmitshur closed this Nov 26, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

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 was marked as resolved.

Copy link
Member

bcmills commented Nov 26, 2018

I'd like Rietveld to mark all the numbers in […] as reserved

Do you mean Gerrit?

@bradfitz

This comment was marked as resolved.

Copy link
Member

bradfitz commented Nov 26, 2018

Yes, fixed.

@bcmills

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

gopherbot commented Jan 9, 2019

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

@dmitshur dmitshur removed the help wanted label Jan 9, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 10, 2019

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 10, 2019

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

@dmitshur dmitshur reopened this Jan 10, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Jan 10, 2019

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.

@dmitshur dmitshur closed this Jan 10, 2019

@adg

This comment has been minimized.

Copy link
Contributor

adg commented Jan 10, 2019

Nicely done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment