searcher: Modernize entrypoint and gRPC server#63700
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d89b72a to
116ccf3
Compare
This PR does a bit of cleanup / alignment with other services, most notably: - Loads config via the config method of the shared routine package plus tests for default config values - Listens on HTTP using the existing httpserver package - Makes it clear what gitserver methods are used by calling them at the entry function instead of passing the whole client down - Adds exhaustive request logging for better auditability (and found an unused field which I removed) - Rename log to logger - Add the common stack of HTTP middlewares for actor, client etc propagation Test plan: Unit tests still pass. CI is green and I was able to run unindexed searches locally.
116ccf3 to
84b2249
Compare
| base proto.SearcherServiceServer | ||
| logger log.Logger | ||
|
|
||
| proto.UnsafeSearcherServiceServer // Consciously opt out of forward compatibility checks to ensure that the go-compiler will catch any breaking changes. |
There was a problem hiding this comment.
Ah, so rather than just responding with an "unimplemented", since this doesn't implement the required interface, it will just fail to compile?
Q: why is it not recommended to do this even for non-wrapper servers? Seems like if we add an RPC to the proto, it would be more useful to fail to compile since these are "canonical" services and we aren't implementing them with multiple backends.
There was a problem hiding this comment.
I have honestly no idea - I copied this 1:1 from the implementation @ggilmore wrote for gitserver exhaustive logging 😬
| type Config struct { | ||
| env.BaseConfig | ||
|
|
||
| ListenAddress string | ||
| CacheDir string | ||
| CacheSizeMB int | ||
| BackgroundTimeout time.Duration | ||
| MaxTotalGitArchivePathsLength int | ||
| DisableHybridSearch bool | ||
| ExhaustiveRequestLoggingEnabled bool | ||
| } |
| // URL specifies the repository's Git remote URL (for gitserver). It is optional. See | ||
| // (gitserver.ExecRequest).URL for documentation on what it is used for. | ||
| URL string |
There was a problem hiding this comment.
Oh nice. I didn't realize this was no longer used. That feels good!
| reserved 6; | ||
| reserved "url"; |
There was a problem hiding this comment.
For my understanding: we reserve both the number and the name because the number is used in the binary encoding and the name is used in the canonical JSON encoding?
There was a problem hiding this comment.
That is my understanding - yes!
There was a problem hiding this comment.
Good to know! Should we have done this for other old fields too (I see reserved: <number> a lot of places already)?
There was a problem hiding this comment.
If we do decide to at some point expose the APIs via some mechanism like connectrpc that MSP services use to expose gRPC to the internet then this would definitely help - but it's only a real problem when some old service might still reference this field, which inside sourcegraph is usually not the case since all services are kept in lockstep
jtibshirani
left a comment
There was a problem hiding this comment.
Nice clean-up with URL :)
| reserved 6; | ||
| reserved "url"; |
There was a problem hiding this comment.
Good to know! Should we have done this for other old fields too (I see reserved: <number> a lot of places already)?
|
|
||
| filter.CommitIgnore = func(hdr *tar.Header) bool { return false } // default: don't filter | ||
| if s.FilterTar != nil { | ||
| filter.CommitIgnore, err = s.FilterTar(ctx, s.GitserverClient, repo, commit) | ||
| if err != nil { | ||
| return nil, errors.Errorf("error while calling FilterTar: %w", err) | ||
| } | ||
| filter.CommitIgnore, err = s.FilterTar(ctx, repo, commit) | ||
| if err != nil { | ||
| r.Close() | ||
| return nil, errors.Errorf("error while calling FilterTar: %w", err) | ||
| } |
There was a problem hiding this comment.
@eseliger do you remember why you made this a required field?
There was a problem hiding this comment.
I didn't see a test that doesn't set it 🤔 So I thought it'd be safe to always call and not have a case here where at runtime we forget to set it and then don't filter by accident. If that breaks anything totally fine to revert
There was a problem hiding this comment.
It broke searcher benchmarks, but no worries fixed forward.
This PR does a bit of cleanup / alignment with other services, most notably:
Test plan:
Unit tests still pass. CI is green and I was able to run unindexed searches locally.