Add Unique Queue infrastructure and move TestPullRequests to this #9856

zeripath · 2020-01-18T15:01:05Z

Add UniqueQueue as a type of Queue
Move TestPullRequests to be a UniqueQueue

modules/queue/unique_queue.go

codecov-io · 2020-01-18T15:40:20Z

Codecov Report

Merging #9856 into master will increase coverage by <.01%.
The diff coverage is 43.44%.

@@            Coverage Diff             @@
##           master    #9856      +/-   ##
==========================================
+ Coverage   43.39%   43.39%   +<.01%     
==========================================
  Files         575      575              
  Lines       79636    79636              
==========================================
+ Hits        34557    34560       +3     
+ Misses      40798    40795       -3     
  Partials     4281     4281

Impacted Files	Coverage Δ
modules/setting/setting.go	`44.07% <0%> (ø)`	⬆️
cmd/embedded_stub.go	`0% <0%> (ø)`	⬆️
modules/repofiles/action.go	`63.45% <100%> (ø)`	⬆️
modules/repofiles/update.go	`39.02% <42.2%> (ø)`	⬆️
routers/private/hook.go	`36.43% <50%> (ø)`	⬆️
models/gpg_key.go	`54.81% <0%> (-0.56%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25c1427...111a87e. Read the comment docs.

zeripath · 2020-01-18T17:07:37Z

There is a slight change of semantics here. The modules/sync.UniqueQueue will block whilst PushFunc(data, fn) is running.

I'm not sure if we want that?

zeripath · 2020-01-18T17:33:06Z

~~oh sigh golangci-lint has disagreed...~~ Done

zeripath · 2020-01-25T18:20:20Z

@guillep2k I've ended up implementing the Flush command - and wiring it in here.

This finally appears to fix the deadlock in the tests. I don't think it's masking the issue - I think rather that the deadlock was happening due to previous tests still running over.

zeripath · 2020-01-26T17:26:32Z

Have forcibly rebased this on #10001

integrations/testlogger.go

6543 · 2020-01-29T12:44:14Z

@zeripath Pleace rebase :)

This adds functionality for Unique Queues

guillep2k · 2020-01-29T23:45:25Z

Is there any particular reason why you didn't implement this as a wrapper that adds uniqueness semantics? Something like (pardon my French):

queue, err := NewLevelQueue(handle, levelCfg, exemplar)
if err != nil && requires_uniqueness {
	queue, err = NewUniqueQueue(queue)
}

I feel like there's a lot of repeated code, and maintenance will be a problem.

zeripath · 2020-01-30T08:02:33Z

You definitely can't just stick a wrapper around the queues as the uniqueness semantics is deep not shallow.

However for level (and possibly redis) I guess I can make an interface that represents a common nosqldb queue - and can have a readToChan (Run/Process?) function that does our work - the trouble is that interface ends up looking a lot like queue.Queue +- Flushable! We need something like this if we want to have share nosqldb connections anyway - as at present our configuration is going to lead to a proliferation of redisclients/leveldbs as more things get moved to queues.

This will not reduce the amount of code but it will reduce the amount of code duplication.

The wrapper is the most excessive reuse of code - some of it might be possible to be moved into delayed starter(?).

modules/setting/queue.go

modules/queue/setting.go

guillep2k · 2020-01-30T23:20:43Z

services/pull/check.go

-// pullRequestQueue represents a queue to handle update pull request tests
-var pullRequestQueue = sync.NewUniqueQueue(setting.Repository.PullRequestQueueLength)
+// prQueue represents a queue to handle update pull request tests
+var prQueue queue.UniqueQueue


I know you wrote about why this required a unique queue somewhere, but I just can't find your comment. Was it because PRs were checked right after being merged in the UI?

We currently request tests on PRs they'd never stop being tested if we didn't unique them.

Do they lock each other and produce a deadlock? Can that happen with two different PRs on the same repo?

It's not that - it's about over testing the patches. They're not free to do!

~~If you update a repo any PR attached to that repo is currently added to the PR patch checking queue.~~ I misremembered this code - I just went to improve that but it's already fine...

However, imagine the Gitea case. Gitea has >100 PR. When you push to master all of those PRs need to be tested again - if the first one tested succeeds and you merge that before the others are complete they need to be tested again... If you are not uniqueing then you're going to be doing ~90 an additional unnecessary tests and that new PR of yours is not going to be tested for 190 tests...

6543

still a lot code :)

guillep2k

Wow!!! Such hard work!! 😮 🚀

It looks much better than previous versions. Here some comments. This time I've finished reading all of it, so pretty please, if possible refrain from force pushing over this commit. 😅 🙏

modules/queue/queue_bytefifo.go

modules/queue/queue_disk_channel.go

modules/queue/unique_queue_disk_channel.go

modules/queue/unique_queue_wrapped.go

modules/setting/queue.go

guillep2k · 2020-02-01T05:08:38Z

services/pull/check.go

-// pullRequestQueue represents a queue to handle update pull request tests
-var pullRequestQueue = sync.NewUniqueQueue(setting.Repository.PullRequestQueueLength)
+// prQueue represents a queue to handle update pull request tests
+var prQueue queue.UniqueQueue


Do they lock each other and produce a deadlock? Can that happen with two different PRs on the same repo?

guillep2k · 2020-02-01T05:21:36Z

modules/queue/unique_queue_channel.go

+	queue.WorkerPool = NewWorkerPool(func(data ...Data) {
+		for _, datum := range data {
+			queue.lock.Lock()
+			delete(queue.table, datum)


The datum is removed from the table before it's handled. checkAndUpdateStatus() call to .Has() becomes pointless.

Are you taking about in check.go?

Yes, this replicates current behaviour. Once you start handling a queue datum you have to assume that any further additions may be because of updates to that datum - hence it should be removed from the unique queue table at that point.

Yeah, but now you're not preventing two checks from running at the same time! 🤔

Hmm... need to think about this one. The logic in the check PR function is pretty bad in any case and needs a refactor. Part of me wonders if we should fix it in another PR.

Right, as I've written before the logic here is broken. We really need your temporary lock thing to do the status changes but I think (!) if we change the following then it should improve things:

gitea/services/pull/check.go

Lines 30 to 42 in 5ab601b

// AddToTaskQueue adds itself to pull request test task queue.

func AddToTaskQueue(pr *models.PullRequest) {

go func() {

err := prQueue.PushFunc(strconv.FormatInt(pr.ID, 10), func() error {

pr.Status = models.PullRequestStatusChecking

err := pr.UpdateCols("status")

if err != nil {

log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err)

} else {

log.Trace("Adding PR ID: %d to the test pull requests queue", pr.ID)

}

return err

})

This needs to only update to StatusChecking if status is not StatusMerged.

Then...

gitea/services/pull/check.go

Lines 199 to 220 in 5ab601b

prID := datum.(string)

id := com.StrTo(prID).MustInt64()

log.Trace("Testing PR ID %d from the pull requests patch checking queue", id)

pr, err := models.GetPullRequestByID(id)

if err != nil {

log.Error("GetPullRequestByID[%s]: %v", prID, err)

continue

} else if pr.Status != models.PullRequestStatusChecking {

continue

} else if manuallyMerged(pr) {

continue

} else if err = TestPatch(pr); err != nil {

log.Error("testPatch[%d]: %v", pr.ID, err)

pr.Status = models.PullRequestStatusError

if err := pr.UpdateCols("status"); err != nil {

log.Error("update pr [%d] status to PullRequestStatusError failed: %v", pr.ID, err)

}

continue

}

checkAndUpdateStatus(pr)

We should only get the PR if the status is not merged.

Then we can drop the check in 208

and in setMerged we need to only update if not already set merged.

…disk

modules/queue/unique_queue_disk_channel.go

modules/queue/queue_bytefifo.go

guillep2k

LGTM. If possible, add some comments clarifying the "uniqueness" of the unique queues to avoid pitfalls in the future.

Kudos for the monstrous work!! 🎉

zeripath · 2020-02-02T23:18:24Z

Make lg-tm work

zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 18, 2020

zeripath added this to the 1.12.0 milestone Jan 18, 2020

lunny reviewed Jan 18, 2020

View reviewed changes

modules/queue/unique_queue.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 18, 2020

zeripath mentioned this pull request Jan 26, 2020

Make WorkerPools and Queues flushable #10001

Merged

zeripath force-pushed the unique-queues branch from dc9d8ff to 2c4350b Compare January 26, 2020 17:26

6543 reviewed Jan 27, 2020

View reviewed changes

integrations/testlogger.go Outdated Show resolved Hide resolved

zeripath force-pushed the unique-queues branch 3 times, most recently from a2f44af to 6ca8430 Compare January 28, 2020 21:51

zeripath added 4 commits January 29, 2020 16:22

Upgrade levelqueue to version 0.2.0

8eb4ce3

This adds functionality for Unique Queues

Add UniqueQueue interface and functions to create them

ca14f21

Add UniqueQueue implementations

22c4563

Move TestPullRequests over to use UniqueQueue

92c2207

zeripath force-pushed the unique-queues branch from 6ca8430 to 92c2207 Compare January 29, 2020 16:23

fix revive

e6bd896

6543 approved these changes Jan 29, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 29, 2020

guillep2k reviewed Jan 31, 2020

View reviewed changes

zeripath added 2 commits January 31, 2020 22:16

Reduce code duplication

7b967e0

Merge remote-tracking branch 'origin/master' into unique-queues

4b278ce

zeripath added 2 commits January 31, 2020 22:17

Merge remote-tracking branch 'origin/master' into unique-queues

486a8e0

Add bytefifos

307f18a

6543 approved these changes Jan 31, 2020

View reviewed changes

guillep2k reviewed Feb 1, 2020

View reviewed changes

zeripath added 8 commits February 1, 2020 09:35

fix locking in persistablechanneluniquequeue shutdown

028872e

rename queue pr_patch_checker

8f6f779

Ensure invalid types are logged

2d88ad7

Move body of ByteFIFOQueue terminate out of select

23e7017

Fix close race in PersistableChannelQueue Shutdown

a2fb188

Fix double lock in unique wrapped

f53c675

Simplify PushFunc in Unique wrapped slightly

c54e2e4

rename q fifo in queue_disk and handle not found err in unique_queue_…

d82abd9

…disk

guillep2k reviewed Feb 1, 2020

View reviewed changes

modules/queue/unique_queue_disk_channel.go Show resolved Hide resolved

modules/queue/queue_bytefifo.go Show resolved Hide resolved

zeripath added 3 commits February 1, 2020 19:05

Lock for Empty and readToChan

47d94a1

Merge branch 'master' into unique-queues

343cded

Merge branch 'master' into unique-queues

5ab601b

guillep2k approved these changes Feb 2, 2020

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 2, 2020

zeripath added 3 commits February 2, 2020 21:57

Add some more comments

25c1427

Merge branch 'master' into unique-queues

111a87e

Merge branch 'master' into unique-queues

6b368d6

zeripath merged commit 2c90338 into go-gitea:master Feb 2, 2020

zeripath deleted the unique-queues branch February 28, 2020 17:19

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Unique Queue infrastructure and move TestPullRequests to this #9856

Add Unique Queue infrastructure and move TestPullRequests to this #9856

zeripath commented Jan 18, 2020

codecov-io commented Jan 18, 2020 •

edited

Loading

zeripath commented Jan 18, 2020

zeripath commented Jan 18, 2020 •

edited

Loading

zeripath commented Jan 25, 2020

zeripath commented Jan 26, 2020

6543 commented Jan 29, 2020

guillep2k commented Jan 29, 2020

zeripath commented Jan 30, 2020

guillep2k Jan 30, 2020

zeripath Jan 31, 2020

guillep2k Feb 1, 2020

zeripath Feb 1, 2020 •

edited

Loading

6543 left a comment

guillep2k left a comment

guillep2k Feb 1, 2020

guillep2k Feb 1, 2020

zeripath Feb 1, 2020

guillep2k Feb 1, 2020

zeripath Feb 1, 2020

zeripath Feb 2, 2020

guillep2k left a comment

zeripath commented Feb 2, 2020

	// AddToTaskQueue adds itself to pull request test task queue.
	func AddToTaskQueue(pr *models.PullRequest) {
	go func() {
	err := prQueue.PushFunc(strconv.FormatInt(pr.ID, 10), func() error {
	pr.Status = models.PullRequestStatusChecking
	err := pr.UpdateCols("status")
	if err != nil {
	log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err)
	} else {
	log.Trace("Adding PR ID: %d to the test pull requests queue", pr.ID)
	}
	return err
	})

	prID := datum.(string)
	id := com.StrTo(prID).MustInt64()

	log.Trace("Testing PR ID %d from the pull requests patch checking queue", id)

	pr, err := models.GetPullRequestByID(id)
	if err != nil {
	log.Error("GetPullRequestByID[%s]: %v", prID, err)
	continue
	} else if pr.Status != models.PullRequestStatusChecking {
	continue
	} else if manuallyMerged(pr) {
	continue
	} else if err = TestPatch(pr); err != nil {
	log.Error("testPatch[%d]: %v", pr.ID, err)
	pr.Status = models.PullRequestStatusError
	if err := pr.UpdateCols("status"); err != nil {
	log.Error("update pr [%d] status to PullRequestStatusError failed: %v", pr.ID, err)
	}
	continue
	}
	checkAndUpdateStatus(pr)

Add Unique Queue infrastructure and move TestPullRequests to this #9856

Add Unique Queue infrastructure and move TestPullRequests to this #9856

Conversation

zeripath commented Jan 18, 2020

codecov-io commented Jan 18, 2020 • edited Loading

Codecov Report

zeripath commented Jan 18, 2020

zeripath commented Jan 18, 2020 • edited Loading

zeripath commented Jan 25, 2020

zeripath commented Jan 26, 2020

6543 commented Jan 29, 2020

guillep2k commented Jan 29, 2020

zeripath commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeripath Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

6543 left a comment

Choose a reason for hiding this comment

guillep2k 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

guillep2k left a comment

Choose a reason for hiding this comment

zeripath commented Feb 2, 2020

codecov-io commented Jan 18, 2020 •

edited

Loading

zeripath commented Jan 18, 2020 •

edited

Loading

zeripath Feb 1, 2020 •

edited

Loading