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

Redesigning async message queues (second attempt) #2159

Merged
merged 17 commits into from Sep 19, 2018

Conversation

Projects
None yet
2 participants
@mapkyca
Collaborator

mapkyca commented Sep 16, 2018

Here's what I fixed or added:

This is a second attempt at redesigning the async message queues. It contains many of the changes from #2157, but has a different approach to dispatching.

In addition to the changes in #2157, I have introduced a queue management CLI command to list and dispatch events, and removed the dispatch API endpoint for the time being (because dispatching expensive events in an api call represents a DoS vector.

The queue service now makes use of the management API to retrieve a list of events pending, as well as to garbage collect, however actual dispatch is made using a system call to the event management tool.

Here's why I did it:

The queue management API calls introduced in the previous attempt are actually quite useful, since for one thing it removes the requirement to maintain an open DB connection.

Dispatching the event using the event management tool now means that we can run the dispatch as a CLI process (keeping the advantages of that), but since it now runs as a separate process, any database connections are single use.

Keeping the management api, and introducing a management tool also means that in future we can rewrite the queue service to be driven by something else - node, whatever.

Closes #2155
Closes #2156

mapkyca added some commits Sep 15, 2018

Introducing Service utility tool
Adding a utility class to hold Service utility tools for generating service call security header.

Refs #2155
Refs #2156
Allow AsynchronousQueue dispatch to be run from the web app
This is to allow the async queue to be executed via an API service call.
Adding sanity checking to AsynchronousQueue dispatch
Previously it was possible to re-dispatch an already dispatched event, if it hadn't yet been garbage collected.
i18n: Updating language file for new strings
Queue service has new strings, so we need to update the gettext file.
Introducing queue management tool
Adding a queue management tool to list and dispatch asynchronous queue events.

This will be used by the event queue service to dispatch events as it makes sense to dispatch queued events as a single dispatch within its own process, rather than maintaining an open database connection.
Use queue management to do the dispatching, rather than calling an AP…
…I endpoint

While a service endpoint is cool, it is problematic in real life as stuff that takes a while and uses a lot of resources can
present a DoS risk. 

The old method of dispatching within a CLI service was good, but maintaining an open connection to the database presented its own problems. 

Performing a dispatch using a separate process, via a system call to a separate Known CLI command, should be more reliable.
Disable event dispatch API service
For now, and until long running and intensive tasks can be safely dispatched via the web service, lets disable the dispatch endpoint.

Dispatching should only be done via the CLI service, although the list management endpoints are still useful as it removes the need for the service thread to maintain a connection to the database.

@mapkyca mapkyca requested a review from benwerd Sep 16, 2018

mapkyca added a commit to mapkyca/idno that referenced this pull request Sep 16, 2018

Bringing cron service in line with queue service
Making similar changes to the cron service as with the event queue service, that is, making use of the management api calls for listing and garbage collection, and the queue management tool for safer dispatch.

Closes idno#2159
@mapkyca

This comment has been minimized.

Show comment
Hide comment
@mapkyca

mapkyca Sep 16, 2018

Collaborator

Bonus, you don't need to restart the processor when you do a code update

Collaborator

mapkyca commented Sep 16, 2018

Bonus, you don't need to restart the processor when you do a code update

@benwerd

This comment has been minimized.

Show comment
Hide comment
@benwerd

benwerd Sep 17, 2018

Member

This and its companion are pretty big PRs - I didn't get a chance to review over the weekend but will tonight.

Member

benwerd commented Sep 17, 2018

This and its companion are pretty big PRs - I didn't get a chance to review over the weekend but will tonight.

@mapkyca

This comment has been minimized.

Show comment
Hide comment
@mapkyca

mapkyca Sep 17, 2018

Collaborator

Second PR also includes this one, so may be the easiest one to review.

Collaborator

mapkyca commented Sep 17, 2018

Second PR also includes this one, so may be the easiest one to review.

@mapkyca mapkyca requested a review from idno/owners Sep 19, 2018

@benwerd

LG assuming the error I pointed out is non-catastrophic, and with a few super-minor changes.

Show resolved Hide resolved ConsolePlugins/EventQueueService/Main.php
Show resolved Hide resolved Idno/Core/AsynchronousQueue.php
Show resolved Hide resolved Idno/Pages/Service/Queues/GC.php

@mapkyca mapkyca merged commit aa6a059 into idno:master Sep 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benwerd

This comment has been minimized.

Show comment
Hide comment
@benwerd

benwerd Sep 19, 2018

Member

@mapkyca did you see. my comments on the review?

Member

benwerd commented Sep 19, 2018

@mapkyca did you see. my comments on the review?

@mapkyca

This comment has been minimized.

Show comment
Hide comment
@mapkyca

mapkyca Sep 19, 2018

Collaborator

Yes, I'm applying the feedback as a separate PR to make management of the merges easier

Collaborator

mapkyca commented Sep 19, 2018

Yes, I'm applying the feedback as a separate PR to make management of the merges easier

@mapkyca mapkyca referenced this pull request Sep 19, 2018

Merged

Update from feedback #2166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment