1624 index large files #3337

ijsnow · 2019-04-10T01:39:42Z

This PR is part of the solution for #1624. We are adding a configuration option called "search.largeFiles" which is a list of glob patterns. Any files matching the patterns in the list will be indexed and searched regardless of the size.

This PR:

adds the setting
adds an internal endpoint that serves this setting for zoekt to consume
uses the setting in searcher

Test plan:

Testing searcher implementation:

Run sourcegraph
Make sure basic text search is ran by disabling indexing "search.index.enabled": false,
Add a large file to a repo. (e.g. ijsnow/tmp)
Make sure we don't search it
Add "search.largeFiles": ["bigfile.txt"] to config
Make sure it is in search results

cmd/searcher/search/store.go

schema/site.schema.json

cmd/searcher/search/store.go

cmd/frontend/internal/httpapi/internal.go

cmd/searcher/search/store.go

ijt · 2019-04-16T03:31:57Z

cmd/frontend/internal/httpapi/internal.go

+	LargeFiles []string
+}
+
+// serveSearchConfiguration is _only_ used by the zoekt index server. Zoekt does


Go doc comments aren't supposed to include markdownish things like foo or bar.
See golang/go#16666 for some discussion about this.

I'm optimizing for reading in sourcegraph. Sourcegraph tooltips render as markdown. See this comment in sourcegraph here.

That's probably a bug.

No, it's a sourcegraph feature.

Yeah lots of people use markdown in godoc even though it isn't supported. Luckily godoc has so little features it doesn't ever conflict.

cmd/frontend/internal/httpapi/internal.go

ijt · 2019-04-16T22:50:23Z

LGTM once the tests pass.

codecov · 2019-04-18T00:31:19Z

Codecov Report

Merging #3337 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted Files	Coverage Δ
cmd/frontend/internal/httpapi/internal.go	`0% <0%> (ø)`	⬆️
cmd/frontend/internal/httpapi/httpapi.go	`19.04% <0%> (-0.23%)`	⬇️
cmd/searcher/search/store.go	`73.78% <92.3%> (+1.16%)`	⬆️
web/src/user/settings/backend.tsx

keegancsmith

I have inline suggestions, but otherwise LGTM.

My only possible concern with this implementation is its a global setting across all repositories. However, that is actually good! We can always add a way to narrow it down to a specific repo in the future.

keegancsmith · 2019-04-18T08:42:11Z

cmd/frontend/internal/httpapi/internal.go

+	LargeFiles []string
+}
+
+// serveSearchConfiguration is _only_ used by the zoekt index server. Zoekt does


Yeah lots of people use markdown in godoc even though it isn't supported. Luckily godoc has so little features it doesn't ever conflict.

keegancsmith · 2019-04-18T09:02:20Z

cmd/searcher/search/store.go

 	// key is a sha256 hash since we want to use it for the disk name
-	h := sha256.Sum256([]byte(string(repo.Name) + " " + string(commit)))
+	h := sha256.Sum256([]byte(string(repo.Name) + " " + string(commit) + " " + string(lfpBytes)))


You can avoid the marshalling and have a simpler implementation:

Suggested change

h := sha256.Sum256([]byte(string(repo.Name) + " " + string(commit) + " " + string(lfpBytes)))

h := sha256.Sum256([]byte(fmt.Sprintf("%q %q %q", repo.Name, commit, conf.Get().SearchLargeFiles)))

Oooooh fancy. Didn't know about %q

keegancsmith · 2019-04-18T09:06:00Z

cmd/searcher/search/store.go

-		if hdr.Size > maxFileSize {
+		// We do not search the content of large files unless they are
+		// whitelisted.
+		if hdr.Size > maxFileSize && !ignoreSizeMax(hdr.Name, conf.Get().SearchLargeFiles) {


you want to use a consistent SearchLargeFiles here, while this can change. Additionally there is an overhead of getting the config, and this is done for each file here.

Good enough suggestion: outside of the loop do largeFiles := conf.Get() and use largeFiles here.
More consistent suggestion: Pass down the largeFiles used in prepareZip.

keegancsmith · 2019-04-18T09:07:48Z

schema/site.schema.json

+      "items": {
+        "type": "string"
+      },
+      "group": "Search"


can you add examples. JSON schema supports the "examples" key.

keegancsmith · 2019-04-18T09:08:59Z

schema/site.schema.json

@@ -22,6 +22,14 @@
      "!go": { "pointer": true },
      "group": "Search"
    },
+    "search.largeFiles": {
+      "description": "A list of file glob patterns where matching files will be indexed and searched regardless of their size.",


Link to the description of the glob format (godoc link is good enough)

ijsnow requested a review from keegancsmith April 10, 2019 01:39

ijsnow force-pushed the 1624-index-large-files branch from e1947d1 to 59b6a1d Compare April 11, 2019 00:50

ijsnow commented Apr 11, 2019

View reviewed changes

cmd/searcher/search/store.go Outdated Show resolved Hide resolved

ijsnow commented Apr 11, 2019

View reviewed changes

cmd/searcher/search/store.go Outdated Show resolved Hide resolved

emidoots reviewed Apr 11, 2019

View reviewed changes

schema/site.schema.json Outdated Show resolved Hide resolved

emidoots reviewed Apr 11, 2019

View reviewed changes

schema/site.schema.json Show resolved Hide resolved

emidoots reviewed Apr 11, 2019

View reviewed changes

cmd/searcher/search/store.go Outdated Show resolved Hide resolved

emidoots reviewed Apr 11, 2019

View reviewed changes

cmd/frontend/internal/httpapi/internal.go Show resolved Hide resolved

ijsnow requested a review from ijt April 11, 2019 21:42

ijsnow marked this pull request as ready for review April 15, 2019 19:39

keegancsmith suggested changes Apr 15, 2019

View reviewed changes

ijsnow mentioned this pull request Apr 16, 2019

syntect server fails with large files #3421

Closed

ijt suggested changes Apr 16, 2019

View reviewed changes

ijsnow force-pushed the 1624-index-large-files branch from 136aa24 to cc184b8 Compare April 16, 2019 22:25

ijsnow mentioned this pull request Apr 16, 2019

Code nav: single-click jump-to-definition #3444

Merged

3 tasks

ijsnow force-pushed the 1624-index-large-files branch from ffb2138 to a8e613d Compare April 17, 2019 22:57

ijsnow added 13 commits April 17, 2019 17:29

add configuration option for searching large files

25ac54d

add internal endpoint that serves search options

b0d6a7c

wip: impl in searcher

47c31fc

not a pointer

3db5d6d

use non-pointer

845935c

clean up

bc2d8b1

actually call watch func

1c6d8c4

use mutex to prevent race conditions

6b3945e

properly handle race condition

514599d

add comment for internal search config endpoint.

acdfa3f

encode large files settings into cache key for better cache invalidation

6a065e2

address review comments

9085721

fix test

87da254

ijsnow force-pushed the 1624-index-large-files branch from a8e613d to 87da254 Compare April 18, 2019 00:31

ijsnow requested a review from keegancsmith April 18, 2019 00:32

keegancsmith approved these changes Apr 18, 2019

View reviewed changes

ijsnow added 2 commits April 18, 2019 08:55

address review comments

65bc9b3

add example and link to settings

744d5d3

ijsnow merged commit 17ac46c into master Apr 18, 2019

ijsnow deleted the 1624-index-large-files branch April 18, 2019 16:34

keegancsmith mentioned this pull request Jun 5, 2019

Indexing doesn't work on specific yarn.lock file, why? #4360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1624 index large files #3337

1624 index large files #3337

ijsnow commented Apr 10, 2019 •

edited

Loading

ijt Apr 16, 2019

ijsnow Apr 16, 2019 •

edited

Loading

ijt Apr 16, 2019

ijsnow Apr 18, 2019

keegancsmith Apr 18, 2019

ijt commented Apr 16, 2019

codecov bot commented Apr 18, 2019 •

edited

Loading

keegancsmith left a comment

keegancsmith Apr 18, 2019

keegancsmith Apr 18, 2019

ijsnow Apr 18, 2019

keegancsmith Apr 18, 2019

keegancsmith Apr 18, 2019

keegancsmith Apr 18, 2019

	h := sha256.Sum256([]byte(string(repo.Name) + " " + string(commit) + " " + string(lfpBytes)))
	h := sha256.Sum256([]byte(fmt.Sprintf("%q %q %q", repo.Name, commit, conf.Get().SearchLargeFiles)))

1624 index large files #3337

1624 index large files #3337

Conversation

ijsnow commented Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

ijsnow Apr 16, 2019 • edited Loading

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

ijt commented Apr 16, 2019

codecov bot commented Apr 18, 2019 • edited Loading

Codecov Report

keegancsmith 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

ijsnow commented Apr 10, 2019 •

edited

Loading

ijsnow Apr 16, 2019 •

edited

Loading

codecov bot commented Apr 18, 2019 •

edited

Loading