Skip to content

Feature/queue performance#5

Open
jacobgardner wants to merge 8 commits intowonderlic:developfrom
jacobgardner:feature/queue-performance
Open

Feature/queue performance#5
jacobgardner wants to merge 8 commits intowonderlic:developfrom
jacobgardner:feature/queue-performance

Conversation

@jacobgardner
Copy link
Copy Markdown
Contributor

This includes my other PR, #4. Created two to keep the review separate.

Things:

  • To build an index, you need to explicitly set the createIndex flag to true on MessageQueue
  • The place I created the index is not optimal, but I think the best place without being a breaking change.

@wonderlic-stephene wonderlic-stephene changed the base branch from master to develop October 9, 2019 23:03
Comment thread lib/message-queue.js Outdated
Comment thread lib/message-queue.js
Comment thread lib/message-queue.js Outdated
Comment thread lib/message-queue.js
async function _startPolling() {
if (!_pollingIntervalId) {
// Temporarily assigning polling interval to prevent multiple polls starting while index is being created
_pollingIntervalId = -1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand this. Does the await on line 93 not actually keep line 97 from starting the polling?

Copy link
Copy Markdown
Contributor Author

@jacobgardner jacobgardner Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to avoid a "race condition" when registerWorker is called twice in quick succession, which is what everyone will probably be doing anyway, including us.

// _pollingIntervalId is undefined
mq.registerWorker('typeOne', (queueItem) => {...});
// _startPolling called => !_pollingIntervalId === true => createIndex awaiting
mq.registerWorker('typeTwo', (queueItem) => {...});
// _startPolling called => !_pollingIntervalId === true => createIndex awaiting again
// first createIndex finishes => _pollingIntervalId updated and setInterval called
// second createIndex finishes => _pollingIntervalId changed (first one lost) and setInterval called again. 

This is a pretty awkward place to be creating the index as it is...
I think I would rather do a major version bump, have the database promise passed in through the constructor and do a createIndex once that gets fulfilled and then start the poller (if register worker has been called).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes more sense now. Let's sleep on it and discuss further tomorrow.

@jacobgardner
Copy link
Copy Markdown
Contributor Author

Updated based on feedback. I need to test these changes still.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants