Various improvements to new indexer gitlab-zoekt-indexer extracted from code review
The following discussions from gitlab-org/search-team/gitlab-zoekt-indexer!12 (merged) should be addressed:
-
@DylanGriffith started a discussion:
Nitpick (non-blocking): I don't know if it's a good idea but I notice that
s.opts.indexDir
will be static and so it kinda feels natural to me that this would be used to initialize a long-lived indexer that's used for all requests and the other arguments are passed toIndexRepository
. This might make it messier now though so I don't think we need to change it. -
@DylanGriffith started a discussion:
It would be nice if there was some way to unit test this.
-
@DylanGriffith started a discussion:
I guess here we could do something like:
s.incrementRequestsTotal(...)
And then check that the number is larger than 0
-
@DylanGriffith started a discussion: (+2 comments)
Nitpick (non-blocking): I wonder if there is a better name other thank
client
here. Technically this whole thing is a Zoekt server so it's kinda weird to seeNewZoektClient
in this code. -
@DylanGriffith started a discussion:
So deleteing just calls
builder.MarkFileAsChangedOrRemoved
but then we don't need to callzoektClient
at all? -
@DylanGriffith started a discussion:
Since we build this interface could we make it simpler for our needs? I would have thought something like
i.zoektClient.UpdateFile
andi.zoektClient.DeleteFile
which also didMarkFileAsChangedOrRemoved
at the same time would be a little neater but maybe there are details I'm missing. -
@DylanGriffith started a discussion: (+1 comment)
Then with gitlab-org/search-team/gitlab-zoekt-indexer!12 (comment 1408515321) this might be like
i.zoektClient.Flush()
if I understand correctly what the idea is here. -
@DylanGriffith started a discussion: (+2 comments)
I wonder if we should start sending the branch name from GitLab now or in a later iteration. At some point the branch name will need to be sent from GitLab when we allow configuring multiple branches for indexing.
-
@DylanGriffith started a discussion:
How exactly do these
branches
work? If we are updating a single branch do we still set all the indexed branches in here or do we only need to list all the branches that we're updating? I find this Zoekt API kinda confusing. -
@DylanGriffith started a discussion: (+1 comment)
Why does it think we have no test coverage for this? Isn't it covered by the integration test?
-
@DylanGriffith started a discussion:
Nitpick (non-blocking): It would be neater if the
zoekt
types never escaped this file so we would never return abuild.Builder
outside of this package. My other suggestion about the API for adding and deleting files maybe covers this.