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.

Sign up for GitHub

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

Debug: make indexing timeout configurable #676

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Debug: make indexing timeout configurable #676

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b72cddc
Debug: make timeout configurable
jtibshirani Oct 30, 2023
433ceab
Read indexing timeout when starting server
jtibshirani Oct 31, 2023
250b6d3
Use gitIndexConfig instead of indexArgs
jtibshirani Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions build/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,8 +1088,11 @@ func (b *Builder) writeShard(fn string, ib *zoekt.IndexBuilder) (*finishedShard,
return nil, err
}

log.Printf("finished %s: %d index bytes (overhead %3.1f)", fn, fi.Size(),
float64(fi.Size())/float64(ib.ContentSize()+1))
log.Printf("finished shard %s: %d index bytes (overhead %3.1f), %d files processed \n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: you don't need the \n with the log pkg.

Suggested change
log.Printf("finished shard %s: %d index bytes (overhead %3.1f), %d files processed \n",
log.Printf("finished shard %s: %d index bytes (overhead %3.1f), %d files processed",

fn,
fi.Size(),
float64(fi.Size())/float64(ib.ContentSize()+1),
ib.NumFiles())

return &finishedShard{f.Name(), fn}, nil
}
Expand Down
12 changes: 8 additions & 4 deletions cmd/zoekt-sourcegraph-indexserver/index.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ import (
sglog "github.com/sourcegraph/log"
)

// indexTimeout defines how long the indexserver waits before
// killing an indexing job.
const indexTimeout = 1*time.Hour + 30*time.Minute // an index should never take longer than an hour and a half
const defaultIndexingTimeout = 1*time.Hour + 30*time.Minute

// IndexOptions are the options that Sourcegraph can set via it's search
// configuration endpoint.
Expand Down Expand Up @@ -183,7 +181,13 @@ func gitIndex(c gitIndexConfig, o *indexArgs, sourcegraph Sourcegraph, l sglog.L

buildOptions := o.BuildOptions()

ctx, cancel := context.WithTimeout(context.Background(), indexTimeout)
// indexingTimeout defines how long the indexserver waits before killing an indexing job.
indexingTimeout := getEnvWithDefaultDuration("INDEXING_TIMEOUT", defaultIndexingTimeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor: I think prefixing the envvar with something can help when reading configuration files. I see we have lost our consistency here, but will still have a lot of envvars prefixed with SRC_. We don't use single container deployments as much these days so likely less important, but the prefix can help with accidental collisions as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make this part of indexArgs and set it in Server.indexArgs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We chatted about this over video, @stefanhengl is okay with an environment variable since we want to be able to change it without bouncing the server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a closer look at this, and decided to go with your suggestion, to match what we do for other debugging options like SKIP_SYMBOLS_REPOS_ALLOWLIST. I think it's best to be consistent, even though that means you need to restart the server to pick up new config.

if indexingTimeout != defaultIndexingTimeout {
debug.Printf("using configured indexing timeout: %s", indexingTimeout)
}

ctx, cancel := context.WithTimeout(context.Background(), indexingTimeout)
defer cancel()

gitDir, err := tmpGitDir(o.Name)
Expand Down
2 changes: 1 addition & 1 deletion cmd/zoekt-sourcegraph-indexserver/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ var (
Help: "A histogram of latencies for indexing a repository.",
Buckets: prometheus.ExponentialBucketsRange(
(100 * time.Millisecond).Seconds(),
(40*time.Minute + indexTimeout).Seconds(), // add an extra 40 minutes to account for the time it takes to clone the repo
(40*time.Minute + defaultIndexingTimeout).Seconds(), // add an extra 40 minutes to account for the time it takes to clone the repo
20),
}, []string{
"state", // state is an indexState
Expand Down
4 changes: 4 additions & 0 deletions gitindex/index.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,15 +529,19 @@ func indexGitRepo(opts Options, config gitIndexConfig) error {

var names []string
fileKeys := map[string][]fileKey{}
totalFiles := 0

for key := range repos {
n := key.FullPath()
fileKeys[n] = append(fileKeys[n], key)
names = append(names, n)
totalFiles++
}

sort.Strings(names)
names = uniq(names)

log.Printf("attempting to index %d total files", totalFiles)
for _, name := range names {
keys := fileKeys[name]

Expand Down
5 changes: 5 additions & 0 deletions indexbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ func (b *IndexBuilder) ContentSize() uint32 {
return b.contentPostings.endByte + b.namePostings.endByte
}

// NumFiles returns the number of files added to this builder
func (b *IndexBuilder) NumFiles() int {
return len(b.contentStrings)
}

// NewIndexBuilder creates a fresh IndexBuilder. The passed in
// Repository contains repo metadata, and may be set to nil.
func NewIndexBuilder(r *Repository) (*IndexBuilder, error) {
Expand Down
2 changes: 1 addition & 1 deletion merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func builderWriteAll(fn string, ib *IndexBuilder) error {
return err
}

log.Printf("finished %s: %d index bytes (overhead %3.1f)", fn, fi.Size(),
log.Printf("finished shard %s: %d index bytes (overhead %3.1f)", fn, fi.Size(),
float64(fi.Size())/float64(ib.ContentSize()+1))

return nil
Expand Down
Loading