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

Add the ability to set a priority #3

Merged
merged 3 commits into from Oct 17, 2017
Merged

Add the ability to set a priority #3

merged 3 commits into from Oct 17, 2017

Conversation

queicherius
Copy link
Contributor

Thanks to the ordering of the file system, by prefixing the filename with a number, we get a priority functionality for (essentially) free.

Thanks to the ordering of the file system, by prefixing the filename
with a number, we get a priority functionality for (essentially) free.
@alexpusch
Copy link
Owner

This is a cool idea but I have some issues with it:

  • We cannot give real assurances that a message with some priority will be processed fully before an other message with a lower priority. This feature can be useful even without a hard assurance but this limitation should be documented.
  • I would rather the message to remain a simple data object without metadata fields. Please pass the priority in options object of publish.

Thanks for the contribution!

@alexpusch
Copy link
Owner

Another issue is that you relay on that fs.readDir will return the files sorted alphanumerically. While this might be true for now I would rather to have an explicit sort in getNextMessageId

@queicherius
Copy link
Contributor Author

queicherius commented Oct 15, 2017

We cannot give real assurances that a message with some priority will be processed fully before an other message with a lower priority. This feature can be useful even without a hard assurance but this limitation should be documented.

Isn't the "reading" part always be in the order of priority (or before this PR in order of timestamp)? I am not quite sure I understand the limitation. Is this about parallel processing, in which case both could start at the same time and the low priority one could finish first?

I would rather the message to remain a simple data object without metadata fields. Please pass the priority in options object of publish.

That's cool, I'll update this PR soon.

Another issue is that you relay on that fs.readDir will return the files sorted alphanumerically. While this might be true for now I would rather to have an explicit sort in getNextMessageId

Damn, I didn't realize that. That indeed is an issue already: 1 - 10 - 3 - 50 is the order on OSX. A possible solution would be limiting the priority to 1-9 and emitting the sorting - that should work with the native alphasort that is used for ordering by timestamp already.

Sorting the files manually seems like a performance killer for many messages to me. Speaking of that tho, it looks like readDir is not sorted on Windows. I also couldn't find a function to just "grab the first filename based on order" instead of the full directory either. 😒


Edit: Tested sorting for 10.000 messages, took about 75ms per sort on average on my machine.

@alexpusch
Copy link
Owner

10,000/75ms might sound fast, but for the current implementation we would need to sort for each consumed message separately. Lets say that more messages are coming it and the queue is steady at 10000. Consuming the first 10000 would take 750 seconds.

There might be some solutions for this -

  1. Split the pending directory into sub-directories by priority, this way we won't have to worry about readDir ordering as we'll read each priority folder individually.
  2. keep the entire list of files in each consumer process and have a file watcher keep track of new files. Inserting a new element into a sorted list is an O(logN) operation.

It seems that it's not 'free' as we previously thought. You are welcome to try to re-implement it, I'll keep this PR open.

@queicherius
Copy link
Contributor Author

queicherius commented Oct 15, 2017

Alright, changes I made:

  • Added "no assurances" into the README
  • Changed the priority to be passed in as an option via publish(message, options) instead of in the message (and moved the default into queuelite instead of storage)
  • Enforced priority to be between 1 and 9. This allows us to get away with not additionally sorting, since it works the same way the current timestamp sorting works (alphabetically sorted with same length for all file names)

This does not solve the issue of readDir not being sorted on Windows, but that's something else to tackle and unrelated to this feature.

lib/storage.js Outdated
const filePath = path.join(this.pendingDir, `${nowNanoSeconds}`);
const filePath = path.join(
this.pendingDir,
`${options.priority}${nowNanoSeconds}`
Copy link
Owner

Choose a reason for hiding this comment

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

Add an _ between to priority and the nanoSeconds. Just for human readability

README.md Outdated
@@ -70,6 +70,7 @@ queue.publish(message);
```
Publishes a new message to the job queue. Returns a promise that resolves when publishing is done
- message - plain js object containing message data
- You can order messages by adding a `_priority` property to the message object (1 - 9). Messages will be consumed in order of priority, lowest first (1, 2, 3...). The default priority is "5". There is no assurance that a message with some priority will be processed fully before an other message with a lower priority, just that they will be read in order of priority.
Copy link
Owner

Choose a reason for hiding this comment

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

You forgot up update the docs about the move of priority into an options object

@queicherius
Copy link
Contributor Author

Fixed both issues. 🙂

@alexpusch alexpusch merged commit 0ae3e5a into alexpusch:master Oct 17, 2017
@alexpusch
Copy link
Owner

Awesome, thanks!

@queicherius queicherius deleted the add-priority branch October 17, 2017 21:55
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.

None yet

2 participants