Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful Queues: Issue Indexing and Tasks #9363

Merged
merged 38 commits into from Jan 7, 2020
Merged

Conversation

@zeripath
Copy link
Contributor

zeripath commented Dec 15, 2019

This PR is a breakout from #8874.

  • modules/queue introduces a generic queue infrastructure with pooled workers, a monitoring page and a generic setting format (This fixes #9004)
  • As a first stage proof of concept we migrate the issues indexer to this format - keeping previous
    config working but allowing it to be overrided and added to in the new queue settings format
  • As a more standard example Tasks are migrated to the new queue format.
@zeripath zeripath added this to the 1.11.0 milestone Dec 15, 2019
@zeripath zeripath mentioned this pull request Dec 15, 2019
20 of 21 tasks complete
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 15, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@f71e1c8). Click here to learn what that means.
The diff coverage is 43.21%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #9363   +/-   ##
=========================================
  Coverage          ?   42.22%           
=========================================
  Files             ?      582           
  Lines             ?    77105           
  Branches          ?        0           
=========================================
  Hits              ?    32555           
  Misses            ?    40551           
  Partials          ?     3999
Impacted Files Coverage Δ
modules/indexer/issues/db.go 54.54% <0%> (ø)
routers/admin/admin.go 11.24% <0%> (ø)
modules/queue/queue_redis.go 1.42% <1.42%> (ø)
modules/setting/setting.go 45.52% <100%> (ø)
routers/routes/routes.go 86.48% <100%> (ø)
modules/queue/queue.go 35.29% <35.29%> (ø)
modules/queue/workerpool.go 41.2% <41.2%> (ø)
modules/indexer/issues/indexer.go 58.37% <41.23%> (ø)
modules/queue/manager.go 41.48% <41.48%> (ø)
modules/queue/setting.go 50% <50%> (ø)
... and 7 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 f71e1c8...0c345db. Read the comment docs.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Dec 18, 2019

I would like to move this to 1.12 since 1.11 has freezed when you send the PR.

Copy link
Member

6543 left a comment

.

Copy link
Member

6543 left a comment

smal format nit

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Dec 22, 2019

@6543 I can't see what the "." and "small format nit" are referring to.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Dec 22, 2019

@lunny I think we do need to put this one in - it's the final step of gracefulising most of the components. It's been part of graceful-2 for months now.

@6543
6543 approved these changes Dec 22, 2019
@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Dec 22, 2019
modules/queue/manager.go Show resolved Hide resolved
modules/queue/manager.go Show resolved Hide resolved
modules/queue/manager.go Outdated Show resolved Hide resolved
modules/queue/queue.go Show resolved Hide resolved
@6543

This comment has been minimized.

Copy link
Member

6543 commented Dec 23, 2019

89b4e04 created a conflict

@zeripath zeripath force-pushed the zeripath:graceful-queues branch from 408d45c to f751693 Dec 23, 2019
@6543

This comment has been minimized.

Copy link
Member

6543 commented Dec 30, 2019

@zeripath I think your rebase messed up :( + conflicting files

zeripath added 10 commits Nov 13, 2019
Remove the old now unused specific queues
@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Jan 4, 2020
@guillep2k

This comment has been minimized.

Copy link
Member

guillep2k commented Jan 4, 2020

That's a lot of work. Thank you!!!

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 4, 2020

@lunny need your review


// Manager is a queue manager
type Manager struct {
mutex sync.Mutex

This comment has been minimized.

Copy link
@lunny

lunny Jan 6, 2020

Member

How about use sync.Map?

This comment has been minimized.

Copy link
@zeripath

zeripath Jan 6, 2020

Author Contributor

The key is a counter - you would need to lock in any case to increment the counter so sync.Map gains you nothing but the loss of static typing.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jan 6, 2020

Need a blank line on these two tables.

图片

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jan 6, 2020

Why we have two task queues? I have no section [task] on my app.ini.

图片

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jan 6, 2020

How about format the timeout as 1s?

 Initial Configuration

{
  "DataDir": "xxxxxxxxx",
  "QueueLength": 20,
  "BatchLength": 20,
  "Workers": 1,
  "MaxWorkers": 10,
  "BlockTimeout": 1000000000,
  "BoostTimeout": 300000000000,
  "BoostWorkers": 5,
  "Name": "issue_indexer"
}
@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 6, 2020

#9363 (comment)

This is a fomantic problem take a look at the admin settings page and everywhere else that uses the same html structure.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 6, 2020

#9363 (comment)

The default queue is a persistable-channel so you have a level queue that exists to read from the level db that was persisted at previous shutdown - it should be empty - and a channel queue that Gitea is using.

At terminate the both queue's workers are stopped and the remaining contents of the channel queue is diverted to the level queue before the level queue is closed.

On startup you get both queues again, only this time there should be some contents in the level queue so its worker will work its way through the queue until that is empty at which point it will have nothing more to do. The channel queue will be active in the meantime.


That being said you should have seen a third queue, one to match the outer container queue - I've just fixed that.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 6, 2020

#9363 (comment)

Hmm... That section comes from:

<pre>{{.Queue.Configuration | JsonPrettyPrint}}

in templates/admin/queue.tmpl

So to render the timeouts correctly we'd need to degenericise that and specifically render each config type. It's possible but a lot of work to get right and keep right ... We don't do that for session providers, or other places that use jsonprettyprint.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jan 7, 2020

QUEUE_NAME on [queue.task] seems not be displayed correctly on monitoring UI.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 7, 2020

QUEUE_NAME is the redis queue name. I can change that to make it clearer of required.

@lunny
lunny approved these changes Jan 7, 2020
lunny and others added 3 commits Jan 7, 2020
This reverts commit 1f83b4f.
@sapk sapk merged commit 62eb1b0 into go-gitea:master Jan 7, 2020
2 checks passed
2 checks passed
approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@zeripath zeripath deleted the zeripath:graceful-queues branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.