Syntactic indexing: enqueuer and scheduler #62485

keynmol · 2024-05-07T11:35:25Z

This PR introduces three main components:

Enqueuer – a thin layer responsible for actually inserting the syntactic indexing records into the database.
Scheduler - a service that
- Identifies repositories that haven't been processed in a while
- Identifies policies that match those repositories (policies that have syntactic indexing enabled)
- Identifies commits that match any of the policies
- And finally, enqueues the jobs to index the discovered repositories and commits
Scheduler job – a periodic routine that triggers Scheduler on with specified interval. This job runs as part of the main Worker service, and only schedules jobs if the experimental syntactic indexing feature is enabled.

Refactoring:

Making some methods public in policies.Service to make it easier to test logic that depends on glob matching of repository names (this matching requires a separate state to be updated)
Extracting some test utilities into a separate package

TODO:

~~[ ] Tests for policy iterator~~ - we're currently investigating whether policy iterator is needed at all, as pagination of policies is in question.
Tests for Scheduler
Wire in experimental feature flag

Test plan

New tests for all components

It initialises a new database which of course doesn't contain any of the test data

keynmol · 2024-05-22T10:42:08Z

internal/codeintel/syntactic_indexing/scheduler_test.go

+	for policyID, patterns := range map[int][]string{
+		1100: {"github.com/*"},
+	} {
+		if err := store.UpdateReposMatchingPatterns(ctx, patterns, policyID, nil); err != nil {
+			t.Fatalf("unexpected error while updating repositories matching patterns: %s", err)
+		}


@varungandhi-src I need to use this method to make sure there's data in database created for policy matcher to pick up a repository based on pattern matching.

To use it I had to make policies/store package non-internal. It's not great for encapsulation, but I'm not aware of an idiomatic Go way to make this work.

An alternative would be to change this particular policy to one matching repository by ID explicitly, slightly weakening the tests.

What do you think?

Idiomatic way would likely be to only expose a public interface with the method (or handful of methods) of interest. Sometimes that can be tricky though.

Sometimes that can be tricky though

Yeah, I think exposing this particular method is not worth it.

I think I can expose a new service whose sole purpose is to be used in the periodic job that updates the repo <--> policy matching cache.

So store will remain internal, and the service will delegate to it. If if works out, I'll extract it to a separate PR.

Thanks to our call, I realised it was much easier than I thought initially: 42022f2

github-actions · 2024-05-23T11:05:42Z

Caution

License checking failed, please read: how to deal with third parties licensing.

github-actions · 2024-05-23T11:32:33Z

Caution

License checking failed, please read: how to deal with third parties licensing.

varungandhi-src

Left some initial comments; will take a deeper look at more of the code shortly.

varungandhi-src · 2024-05-23T10:04:45Z

cmd/worker/shared/init/db/db.go

+func InitRawDB(observationCtx *observation.Context) (*sql.DB, error) {
+	rawDB, err := initDatabaseMemo.Init(observationCtx)
+	if err != nil {
+		return nil, err
+	}
+
+	return rawDB, err
+}


Suggested change

func InitRawDB(observationCtx *observation.Context) (*sql.DB, error) {

rawDB, err := initDatabaseMemo.Init(observationCtx)

if err != nil {

return nil, err

}

return rawDB, err

}

func InitRawDB(observationCtx *observation.Context) (*sql.DB, error) {

return initDatabaseMemo.Init(observationCtx)

}

I'm a bit confused though. Why does this need to be called separately/why is InitDB not called during the normal initialization?

varungandhi-src · 2024-05-23T10:17:18Z

internal/codeintel/syntactic_indexing/enqueuer.go

+type EnqueueOptions struct {
+	force       bool
+	bypassLimit bool
+}


Please add doc comments for these fields.

varungandhi-src · 2024-05-23T10:18:39Z

internal/codeintel/syntactic_indexing/enqueuer.go

+}
+
+type operations struct {
+	queueIndex *observation.Operation


Suggested change

queueIndex *observation.Operation

queueIndexes *observation.Operation

nit: Following existing conventions, this should match the method name exactly

varungandhi-src · 2024-05-23T10:18:53Z

internal/codeintel/syntactic_indexing/enqueuer.go

+	}
+
+	return &operations{
+		queueIndex: op("QueueIndex"),


Suggested change

queueIndex: op("QueueIndex"),

queueIndexes: op("QueueIndexes"),

varungandhi-src · 2024-05-23T10:23:53Z

internal/codeintel/syntactic_indexing/enqueuer.go

+	if err != nil {
+		return nil, errors.Wrap(err, "gitserver.ResolveRevision")
+	}
+	commit := string(commitID)


Let's delay converting this to a string until the very end. The api.CommitID type says it is a 40-character SHA, that's useful to know compared to just the name commit.

varungandhi-src · 2024-05-23T11:15:59Z

internal/codeintel/syntactic_indexing/internal/policy_iterator.go

+	offset := 0
+
+	for {
+
+		options := policiesshared.GetConfigurationPoliciesOptions{
+			RepositoryID:         p.RepositoryID,
+			ForSyntacticIndexing: forSyntacticIndexing,
+			ForPreciseIndexing:   forPreciseIndexing,
+			Limit:                p.BatchSize,
+			Offset:               offset,
+		}
+
+		policies, totalCount, err := p.Service.GetConfigurationPolicies(ctx, options)
+
+		if err != nil {
+			return err
+		}
+
+		if len(policies) == 0 {
+			break
+		}
+
+		handlerError := handle(policies)
+
+		if handlerError != nil {
+			return handlerError // propagate error from the handler
+		}
+
+		offset = offset + len(policies)
+
+		if offset >= totalCount {
+			break
+		}
+	}


Suggested change

offset := 0

for {

options := policiesshared.GetConfigurationPoliciesOptions{

RepositoryID: p.RepositoryID,

ForSyntacticIndexing: forSyntacticIndexing,

ForPreciseIndexing: forPreciseIndexing,

Limit: p.BatchSize,

Offset: offset,

}

policies, totalCount, err := p.Service.GetConfigurationPolicies(ctx, options)

if err != nil {

return err

}

if len(policies) == 0 {

break

}

handlerError := handle(policies)

if handlerError != nil {

return handlerError // propagate error from the handler

}

offset = offset + len(policies)

if offset >= totalCount {

break

}

}

options := policiesshared.GetConfigurationPoliciesOptions{

RepositoryID: p.RepositoryID,

ForSyntacticIndexing: forSyntacticIndexing,

ForPreciseIndexing: forPreciseIndexing,

Limit: p.BatchSize,

}

for offset := 0; ; {

options.Offset = offset

policies, totalCount, err := p.Service.GetConfigurationPolicies(ctx, options)

if err != nil {

return err

}

if len(policies) == 0 {

break

}

if handlerError := handle(policies); handlerError != nil {

return handlerError

}

if offset = offset + len(policies); offset >= totalCount {

break

}

}

nit: Please don't add whitespace after every statement.

varungandhi-src · 2024-05-23T11:23:14Z

internal/codeintel/syntactic_indexing/jobstore/store.go

+	InsertIndexes(ctx context.Context, indexes []SyntacticIndexingJob) ([]SyntacticIndexingJob, error)
+	IsQueued(ctx context.Context, repositoryID int, commit string) (bool, error)


Thoughts on pushing this logic into a single method? Otherwise, this API creates the risk of TOCTOU because something might get queued in-between the IsQueued check and the Insert check. AFAICT, that's impossible given that these are currently invoked from the same goroutine, and there's only going to be one such goroutine, and the worst case scenario is that there will be some redundant work, but it seems unnecessarily error-prone.

varungandhi-src · 2024-05-23T11:25:15Z

internal/codeintel/syntactic_indexing/jobstore/store.go

+		// TODO: add operations and record this operation
+		// s.operations.indexesInserted.Add(float64(len(ids)))


Does this need fixing?

varungandhi-src · 2024-05-23T11:33:37Z

internal/codeintel/syntactic_indexing/jobstore/store.go

+		ids, err := basestore.ScanInts(tx.Query(ctx, sqlf.Sprintf(insertIndexQuery, sqlf.Join(values, ","))))
+		if err != nil {
+			return err
+		}
+		// TODO: add operations and record this operation
+		// s.operations.indexesInserted.Add(float64(len(ids)))
+
+		authzConds, err := database.AuthzQueryConds(ctx, database.NewDBWith(s.logger, s.db))
+		if err != nil {
+			return err
+		}
+
+		queries := make([]*sqlf.Query, 0, len(ids))
+		for _, id := range ids {
+			queries = append(queries, sqlf.Sprintf("%d", id))
+		}
+
+		indexes, err = scanIndexes(tx.Query(ctx, sqlf.Sprintf(getIndexesByIDsQuery, sqlf.Join(queries, ", "), authzConds)))
+		return err


I don't understand this code. It looks like we're performing insertions into syntactic_scip_indexing_jobs and returning the ids for the just-inserted rows. Then we're reading more data from those rows in a separate query (through a view). Can we expand the RETURNING clause to return more columns during insertion? I don't understand what role the extra authzConds is playing here.

internal/codeintel/syntactic_indexing/jobstore/store.go

cla-bot bot added the cla-signed label May 7, 2024

github-actions bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels May 7, 2024

keynmol added 8 commits May 13, 2024 12:10

WIP: Syntactic enqueuer

e1b62e8

Add observability

ebbd351

Tests for syntactic indexing enqueuer

d120724

Syntactic indexing scheduler and enqueuer

81f16d2

Separate scheduler into a testable interface

5c244af

Allow injecting raw database to aid with testing

656ec9d

Wire in metrics

5f26d6e

WIP

03414ce

keynmol force-pushed the syntactic-indexing-enqueuer branch from 11d5272 to 03414ce Compare May 13, 2024 11:11

keynmol added 4 commits May 13, 2024 12:24

Wire scheduling job and check site config

26a138b

Enqueuer test and refactor test helpers

eb8e860

Refactor config passing to ensure we don't use default values

58102a0

Remove usage of global InitServices

d92c644

It initialises a new database which of course doesn't contain any of the test data

keynmol changed the title ~~Syntactic indexing: enqueuer~~ Syntactic indexing: enqueuer and scheduler May 20, 2024

keynmol added 3 commits May 22, 2024 10:47

Cleanup scheduler bootstrap and move towards working tests

6797dbc

Make policies store a non-internal module to use from tests

c5a2f87

Add more tests to scheduler

07e823f

keynmol commented May 22, 2024

View reviewed changes

keynmol added 2 commits May 23, 2024 09:49

Restore policy store to internal package

bc31f76

Rejig Policy service interface to allow using it in tests

42022f2

Merge branch 'main' into syntactic-indexing-enqueuer

7c4030e

keynmol marked this pull request as ready for review May 23, 2024 11:30

varungandhi-src reviewed May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syntactic indexing: enqueuer and scheduler #62485

Syntactic indexing: enqueuer and scheduler #62485

keynmol commented May 7, 2024 •

edited

keynmol May 22, 2024

varungandhi-src May 22, 2024

keynmol May 22, 2024

keynmol May 23, 2024

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

varungandhi-src left a comment

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

varungandhi-src May 23, 2024

	queueIndex *observation.Operation
	queueIndexes *observation.Operation

	queueIndex: op("QueueIndex"),
	queueIndexes: op("QueueIndexes"),

		InsertIndexes(ctx context.Context, indexes []SyntacticIndexingJob) ([]SyntacticIndexingJob, error)
		IsQueued(ctx context.Context, repositoryID int, commit string) (bool, error)

		// TODO: add operations and record this operation
		// s.operations.indexesInserted.Add(float64(len(ids)))

Syntactic indexing: enqueuer and scheduler #62485

Are you sure you want to change the base?

Syntactic indexing: enqueuer and scheduler #62485

Conversation

keynmol commented May 7, 2024 • edited

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

varungandhi-src left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keynmol commented May 7, 2024 •

edited