cloud: use the cloned column to filter by clone status #11932

asdine · 2020-07-03T12:26:30Z

This makes the frontend store know about the new cloned column and add filtering capabilities based on its value. It also removes the application level filtering done in by the repositoryResolver which used to ask Gitserver for clone status, and uses the database instead.

Related to #11029

cmd/frontend/graphqlbackend/repositories.go

codecov · 2020-07-03T12:29:43Z

Codecov Report

Merging #11932 into master will decrease coverage by 2.24%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master   #11932      +/-   ##
==========================================
- Coverage   50.13%   47.89%   -2.25%     
==========================================
  Files        1516     1412     -104     
  Lines       88577    80238    -8339     
  Branches     6664     6764     +100     
==========================================
- Hits        44412    38428    -5984     
+ Misses      40221    38216    -2005     
+ Partials     3944     3594     -350

Flag	Coverage Δ
#go	`52.08% <92.00%> (-2.47%)`	⬇️
#storybook	`11.66% <ø> (ø)`
#typescript	`36.78% <ø> (ø)`
#unit	`47.39% <92.00%> (-2.30%)`	⬇️

Impacted Files	Coverage Δ
cmd/frontend/graphqlbackend/repository.go	`25.88% <ø> (-7.06%)`	⬇️
cmd/repo-updater/repos/store.go	`86.17% <77.77%> (-0.39%)`	⬇️
cmd/frontend/db/repos.go	`72.43% <100.00%> (-5.09%)`	⬇️
cmd/frontend/graphqlbackend/repositories.go	`41.66% <100.00%> (+4.01%)`	⬆️
cmd/repo-updater/repos/observability.go	`92.99% <100.00%> (ø)`
cmd/repo-updater/repos/testing.go	`81.41% <100.00%> (-3.28%)`	⬇️
cmd/frontend/db/orgs_mock.go	`0.00% <0.00%> (-100.00%)`	⬇️
cmd/frontend/db/users_mock.go	`0.00% <0.00%> (-100.00%)`	⬇️
internal/extsvc/github/codehost.go	`0.00% <0.00%> (-100.00%)`	⬇️
internal/extsvc/gitlab/codehost.go	`0.00% <0.00%> (-100.00%)`	⬇️
... and 263 more

tsenart

You should test these changes end to end manually too, look at the progress status indicator and the site admin repositories listing pages with the different clone filters.

cmd/frontend/graphqlbackend/repositories.go

tsenart · 2020-07-03T14:27:08Z

cmd/repo-updater/repos/store.go

@@ -739,6 +750,7 @@ SELECT
  external_service_id,
  external_id,
  archived,
+  cloned,


I think that this is OK, since the Update call in NewDiff by the Syncer doesn't care about the cloned field, so it won't reset the value to the default of the src version produce by each Source.

cmd/frontend/graphqlbackend/repositories.go

eseliger

Approving for code owned by campaigns.

cmd/frontend/db/repos_db_test.go

cmd/frontend/graphqlbackend/repositories.go

cmd/frontend/graphqlbackend/repositories_test.go

cmd/repo-updater/shared/main.go

mrnugget

Left some comments — most important one is to add some tests for the boolean fields being positive :)

cmd/frontend/graphqlbackend/repositories_test.go

cmd/repo-updater/repos/observability.go

cmd/repo-updater/repos/store_test.go

cmd/frontend/db/repos_db_test.go

mrnugget · 2020-07-09T07:54:18Z

cmd/frontend/graphqlbackend/repositories.go

-			// As for the number: 1250 is the result of local benchmarks where
-			// it yielded the best performance/resources tradeoff, before
-			// diminishing returns set in
-			opt2.Limit += 1250


Farewell and audieu, long comment and weird magic number! I can't say I'm proud to have added you, but... well, let's say it was interesting and I'm not sad to see you go! 👋

asdine · 2020-07-09T08:47:50Z

cmd/repo-updater/repos/store.go

 	RETURNING id
 )
 UPDATE repo SET cloned = false
 FROM c
- WHERE cloned AND repo.id != c.id;
+ WHERE cloned AND repo.id NOT IN (SELECT id FROM c);


I am trying to update the repo table by setting all of the repos cloned column to false, except the ones that are passed in the IN clause, all of this while limiting the number of writes.
I am not sure this is the best way to do this.
@tsenart @mrnugget @unknwon What do you think?

How about this? Haven't tested it, but I think it goes in the right direction.

WITH names AS ( SELECT UNNEST(%s) AS name ), cloned AS ( UPDATE repo SET cloned = true FROM names WHERE NOT cloned AND name = names.name ) UPDATE repo SET cloned = false FROM names WHERE cloned AND name != names.name;

tsenart

Approving to unblock, no need to ask for review again.

tsenart · 2020-07-09T08:47:14Z

cmd/repo-updater/repos/store.go

 	RETURNING id
 )
 UPDATE repo SET cloned = false
 FROM c
- WHERE cloned AND repo.id != c.id;
+ WHERE cloned AND repo.id NOT IN (SELECT id FROM c);


What is the difference between repo.id != c.id and NOT IN (SELECT id FROM c)?

Apparently, repo.id != c.id only works on the first row of c, not on all the rows. I had to use NOT IN (SELECT id FROM c) to make it work as intended

Ah gotcha, that is surprising, TIL :) So you 'd have to adapt my proposed query.

tsenart · 2020-07-09T08:55:48Z

cmd/repo-updater/repos/store.go

@@ -457,12 +431,26 @@ const setClonedReposQueryFmtstr = `
 -- source: cmd/repo-updater/repos/store.go:DBStore.SetClonedRepos
 WITH c AS (
 	UPDATE repo SET cloned = true
-	WHERE NOT cloned AND name in (%s)
+	WHERE name IN (%s)


Unfortunately this brings us back to the performance issue of updating rows that didn't change. Can you think of a way to avoid that?

Second thing I just thought of: Postgres as limit of 32767 bind parameters per statement. In a large installation, we'd have more repos than that limit. We should be able to circumvent that by passing in a JSON array with the names and using jsonb_array_elements_text to convert that to a set of rows.

Unfortunately this brings us back to the performance issue of updating rows that didn't change. Can you think of a way to avoid that?

I will try yes 👍

Second thing I just thought of: Postgres as limit of 32767 bind parameters per statement. In a large installation, we'd have more repos than that limit. We should be able to circumvent that by passing in a JSON array with the names and using jsonb_array_elements_text to convert that to a set of rows.

Good point

tsenart · 2020-07-09T09:14:31Z

@asdine: Once the overall issue is done, please add a CHANGELOG entry and tell @uwedeportivo we can roll this out to certain affected customers.

cmd/repo-updater/repos/store.go

Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>

uwedeportivo · 2020-07-10T15:28:35Z

@asdine got it, thanks

This reverts commit cab1a64.

…" (#12128) This reverts commit cab1a64.

#11932)" (#12128)" This reverts commit 52130ea.

…s"" (#12149) * Revert "Revert "cloud: use the cloned column to filter by clone status (#11932)" (#12128)" This reverts commit 52130ea. * Fix cloned repositories listing and counting (#12152)

…s"" (#12149) This is cherry-pick for the 3.18.0 release * Revert "Revert "cloud: use the cloned column to filter by clone status (#11932)" (#12128)" This reverts commit 52130ea. * Fix cloned repositories listing and counting (#12152)

asdine requested a review from slimsag as a code owner July 3, 2020 12:26

asdine requested a review from a team July 3, 2020 12:26

asdine commented Jul 3, 2020

View reviewed changes

cmd/frontend/graphqlbackend/repositories.go Outdated Show resolved Hide resolved

tsenart approved these changes Jul 3, 2020

View reviewed changes

Base automatically changed from cloud/add-count-cloned-repos to master July 6, 2020 10:37

asdine requested a review from a team as a code owner July 7, 2020 16:15

eseliger approved these changes Jul 7, 2020

View reviewed changes

asdine requested a review from tsenart July 8, 2020 13:08

unknwon reviewed Jul 9, 2020

View reviewed changes

cmd/frontend/db/repos_db_test.go Outdated Show resolved Hide resolved

cmd/frontend/graphqlbackend/repositories.go Show resolved Hide resolved

cmd/frontend/graphqlbackend/repositories_test.go Outdated Show resolved Hide resolved

cmd/repo-updater/shared/main.go Show resolved Hide resolved

mrnugget approved these changes Jul 9, 2020

View reviewed changes

asdine commented Jul 9, 2020

View reviewed changes

tsenart approved these changes Jul 9, 2020

View reviewed changes

asdine commented Jul 9, 2020

View reviewed changes

cmd/repo-updater/repos/store.go Show resolved Hide resolved

asdine and others added 14 commits July 10, 2020 12:00

Filter by cloned column in repo-updater

1d0ba28

Filter by cloned column in frontend store

30a646b

Use cloned status filters in repositoryConnectionResolver

19d323a

Delete debug log

c2c0cc6

Simplify main path

4770af4

Fix query

cf378ac

Apply suggestions from code review

363c1b0

Co-authored-by: ᴜɴᴋɴᴡᴏɴ <joe@sourcegraph.com>

Clarify obscure condition

e68b0e2

Rewrite tests into table driven tests

c22e255

Apply suggestions from code review

e7da304

Co-authored-by: Thorsten Ball <mrnugget@gmail.com>

Copy Archived bool in createRepo

b20d704

Add explicit test for cloned and notCloned arguments

f8f2d23

Improve SetClonedRepos query

17154df

Fix indentation

49aa369

asdine force-pushed the cloud/filter-by-cloned-status branch from d12e080 to 49aa369 Compare July 10, 2020 08:03

Add entry to the changelog

237d891

asdine merged commit cab1a64 into master Jul 10, 2020

asdine deleted the cloud/filter-by-cloned-status branch July 10, 2020 08:39

asdine mentioned this pull request Jul 10, 2020

repository status page with "cloning" filter is extremely slow #11029

Closed

This was referenced Jul 13, 2020

Delete cloning tab from the site-admin repositories page #12043

Merged

Revert "Delete cloning tab from the site-admin repositories page" #12127

Merged

asdine added a commit that referenced this pull request Jul 13, 2020

Revert "cloud: use the cloned column to filter by clone status (#11932)"

b2fca4b

This reverts commit cab1a64.

asdine mentioned this pull request Jul 13, 2020

Revert "cloud: use the cloned column to filter by clone status" #12128

Merged

asdine added a commit that referenced this pull request Jul 13, 2020

Revert "cloud: use the cloned column to filter by clone status (#11932)…

52130ea

…" (#12128) This reverts commit cab1a64.

asdine added a commit that referenced this pull request Jul 14, 2020

Revert "Revert "cloud: use the cloned column to filter by clone status (

8893e06

#11932)" (#12128)" This reverts commit 52130ea.

asdine restored the cloud/filter-by-cloned-status branch July 14, 2020 08:37

This was referenced Jul 14, 2020

Revert "Revert "cloud: use the cloned column to filter by clone status"" #12149

Merged

Fix cloned repositories listing and counting #12152

Merged

asdine added a commit that referenced this pull request Jul 15, 2020

Revert "Revert "cloud: use the cloned column to filter by clone status (

24bdd88

#11932)" (#12128)" This reverts commit 52130ea.

asdine deleted the cloud/filter-by-cloned-status branch January 15, 2021 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud: use the cloned column to filter by clone status #11932

cloud: use the cloned column to filter by clone status #11932

asdine commented Jul 3, 2020 •

edited

codecov bot commented Jul 3, 2020 •

edited

tsenart left a comment

tsenart Jul 3, 2020

eseliger left a comment

mrnugget left a comment

mrnugget Jul 9, 2020

asdine Jul 9, 2020 •

edited

tsenart Jul 9, 2020

tsenart left a comment

tsenart Jul 9, 2020

asdine Jul 9, 2020 •

edited

tsenart Jul 9, 2020

tsenart Jul 9, 2020

asdine Jul 9, 2020

tsenart commented Jul 9, 2020

uwedeportivo commented Jul 10, 2020

cloud: use the cloned column to filter by clone status #11932

cloud: use the cloned column to filter by clone status #11932

Conversation

asdine commented Jul 3, 2020 • edited

codecov bot commented Jul 3, 2020 • edited

Codecov Report

tsenart left a comment

Choose a reason for hiding this comment

tsenart Jul 3, 2020

Choose a reason for hiding this comment

eseliger left a comment

Choose a reason for hiding this comment

mrnugget left a comment

Choose a reason for hiding this comment

mrnugget Jul 9, 2020

Choose a reason for hiding this comment

asdine Jul 9, 2020 • edited

Choose a reason for hiding this comment

tsenart Jul 9, 2020

Choose a reason for hiding this comment

tsenart left a comment

Choose a reason for hiding this comment

tsenart Jul 9, 2020

Choose a reason for hiding this comment

asdine Jul 9, 2020 • edited

Choose a reason for hiding this comment

tsenart Jul 9, 2020

Choose a reason for hiding this comment

tsenart Jul 9, 2020

Choose a reason for hiding this comment

asdine Jul 9, 2020

Choose a reason for hiding this comment

tsenart commented Jul 9, 2020

uwedeportivo commented Jul 10, 2020

asdine commented Jul 3, 2020 •

edited

codecov bot commented Jul 3, 2020 •

edited

asdine Jul 9, 2020 •

edited

asdine Jul 9, 2020 •

edited