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/cmd/godoc: frequent “Indexing in progress” failures #24965

Closed
bcmills opened this issue Apr 20, 2018 · 14 comments

Comments

Projects
None yet
9 participants
@bcmills
Copy link
Member

commented Apr 20, 2018

On some large fraction of queries to https://golang.org/search, I get an empty results page like the one shown below. If I reload, the error goes away.

It's the server's responsibility to find data to serve: we shouldn't push that off on the user. If indexing usually finishes quickly, we should just wait to serve the page until it's ready. Otherwise, we should prefer to serve stale data instead of an unhelpful error.

screenshot 2018-04-20 at 01 38 33

@bcmills bcmills added this to the Unreleased milestone Apr 20, 2018

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2018

@ChrisALiles

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

I’ve also been seeing this more frequently lately and would appreciate a fix.

@stamblerre

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

I'm actually not familiar with this codebase, but I can look into it.

@stamblerre stamblerre self-assigned this May 1, 2018

@meirf

This comment has been minimized.

Copy link
Contributor

commented May 22, 2018

@stamblerre I was looking around this code for an unrelated search proposal I'm working on so I figured I'd post some notes to make myself useful in case my proposal falters.

I believe the message is set here:

if ts := c.FSModifiedTime(); timestamp.Before(ts) {
    // The index is older than the latest file system change under godoc's observation.
    result.Alert = "Indexing in progress: result may be inaccurate"
}

It looks like a reindex happens every 5 minutes: initialization (not set), usage (use default). This is actually surprising to me since I recall getting the indexing message multiple times in a 10 minute window in what I remember to be off hours. So it's hard to believe that the filesystem is actually changing. Looks like fsModified is only set by invalidateIndex which is called by initFSTree which is called by RunIndexer in every iteration of the forever loop. This tells me that it's not actually based on when fs changes. In any case, you can get more data and maybe prove me wrong by running in verbose mode and seeing timestamps from UpdateIndex's logs and seeing if there were any commits between last index and the time you get the indexing alert.


Otherwise, we should prefer to serve stale data instead of an unhelpful error.

That might just be removing the if code above.

If indexing usually finishes quickly, we should just wait to serve the page until it's ready.

Maybe use a channel which UpdateIndex/RunIndexer sends on and then if it sees index in progress Lookup selects on that channel and a short timeout .

@agnivade

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

This tells me that it's not actually based on when fs changes.

That is correct. There was a separate issue (#7524) which tracked changing this to respond to filesystem changes. But that was closed deciding that controlling the index_interval with the flag is good enough.

That might just be removing the if code above.

Yes, looks like it. I agree that we should just return stale data instead of returning an error.

But then, this is for golang.org. So when it is deployed, it just indexes the standard library. There is no user code here. So, essentially nothing ever changes and we are just re-indexing anyways.

I would suggest setting the index_interval to 1 week or something. So that it effectively becomes a non-issue.

/cc @andybons

@agnivade agnivade changed the title golang.org/search: frequent “Indexing in progress” failures x/tools/cmd/godoc: frequent “Indexing in progress” failures Oct 3, 2018

@stamblerre stamblerre removed their assignment Oct 3, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

/cc @broady who was just playing around with godoc.

@agnivade

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Was going through the godoc flags today and realised that a negative index_interval just indexes once, effectively disabling indexing after that. I think we should set that for golang.org.

@broady

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Closing. Please re-open if you see this on golang.org again.

Indexing happens as part of the deployment process, so this message shouldn't ever be seen.

Edit: shouldn't have been so hasty to close. There's a minor race condition.

RunIndexer is called in a goroutine, so between the first call to c.UpdateIndex() and the sever starting, users will see this message.

With the move to flex, this won't be seen almost at all in practice, but on the old deployment which was OOMing, basically only a single request would be handled before OOM so the chances of seeing this error was high.

@broady broady closed this Oct 18, 2018

@broady broady reopened this Oct 18, 2018

@broady broady self-assigned this Oct 18, 2018

@agnivade

This comment has been minimized.

Copy link
Member

commented Nov 10, 2018

@broady - I mean it's not a race condition "per se". Because the access is mutex protected.

Still, this c.readIndex should happen only once when godoc starts. So even if users see the message, it should be for a very thin window, i.e. the time it takes to read the index file. The issue was about "frequent" indexing failures, which I don't think should be happening now.

I don't see any obvious way around this, unless you want to make c.UpdateIndex() synchronous ?

@broady

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

It's a race between c.UpdateIndex and the first incoming request to the search page.

Yes, in production mode, UpdateIndex should be synchronous (i.e., happen before incoming requests are handled).

That said, it doesn't really matter very much (see the last para in my previous comment).

@gopherbot

This comment has been minimized.

Copy link

commented Nov 12, 2018

Change https://golang.org/cl/148998 mentions this issue: cmd/godoc: start RunIndexer synchronously

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

@broady, do we still ship a pre-built index for golang.org on Flex?

@broady

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Nov 20, 2018

Change https://golang.org/cl/150685 mentions this issue: [release-branch.go1.11] cmd/godoc: start RunIndexer synchronously when index is present

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.