Add ability to cancel a non inflight message

[samgurtman-zz]

We need the ability to remove a non processed message from the queue by the id returned when it was created. We could do ignore it via a blacklist but this seems like a cleaner solution. I'm happy to discuss modifications if so desired.

[chilts]

Maybe @aidenkeating has an opinion here. I personally would probably be against this.

What is your use-case whereby you want to delete a message almost as soon as it has been added?

[samgurtman-zz]

The messages we insert have very long delays (months). We are using it to send notifications of impending due dates. Sometimes those due dates change, and we need to prevent the previously set notifications from going out.

[chilts]

Interesting. I see what you mean that such a long delay might require a cancellation. I'll take a look at your PR and make a decision later. Thanks for your contribution.

[aidenkeating]

@chilts I think I would be for this change, I could see it being useful. @samgurtman I have some suggestions though, which I think could make this feature a bit more useful.

In terms of the implementation, I wonder whether there should be a way of distinguishing between cancelled messages and normally acknowledged messages? Perhaps by adding a boolean cancelled key to the MongoDB record when you're cancelling.

This way, if you're monitoring statistics of your queue for how many jobs your processing every period of time, you won't be seeing artificially high values because you've been cancelling a lot of jobs. I hope that makes sense.

I've made one small comment that I noticed. I'll take a closer look later if @chilts doesn't beat me to it.

[samgurtman-zz]

The problem is the queue itself is not providing that ObjectId in its callbacks but rather the String representation of that. MongoDB is already a devDependency. The alternative is creating a separate ack for cancellation that passed into the add callback along with or instead of the _id.

On 15/03/2017 12:03 AM, "Aiden Keating" notifications@github.com wrote:

@aidenkeating commented on this pull request.

In mongodb-queue.js https://github.com/chilts/mongodb-queue/pull/19#discussion_r105878443:

@@ -265,3 +266,25 @@ Queue.prototype.done = function(callback) { callback(null, count) }) } + +Queue.prototype.cancel = function(id, callback) {

• var self = this
• var query = {
• _id : new ObjectID(id),

@samgurtman https://github.com/samgurtman Sorry for the wait, I've only started looking at this now. I wouldn't be in favour of adding the mongodb dependency to achieve this. IMO a user should pass in an ObjectID object for the id parameter here if they need to cancel.

@chilts https://github.com/chilts May have a different view here though.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chilts/mongodb-queue/pull/19#pullrequestreview-26776418, or mute the thread https://github.com/notifications/unsubscribe-auth/AGIKX8yfJY0Xv0j25wfyKn48KwRLzDb9ks5rlnQYgaJpZM4Ma0h5 .

[aidenkeating]

@samgurtman Yes, we currently have mongodb as a devDependency, but not a regular dependency. We know the user already dependency the mongodb node driver don't we? So I think we should take advantage of that instead of actually depending on mongodb.

I think it would be nicer to have the end user perform the string to ObjectID conversion and avoid depending on mongodb for now. Something like below.

However, in the broader scope of things, maybe a queue option to make the queue return ObjectID's instead of string representations would be a good change. Or just a backwards incompatible change.

wdyt?

var mongo = require('mongodb');
var mongoQueue = require('mongodb-queue');
var ObjectID = mongo.ObjectID;

mongo.connect(mongodbUrl, function(err, db) {
  var queue = mongoQueue(db, 'my-queue');
  queue.add({ test: 'test'}, function(err, id) {
    var toCancel = new ObjectID(id);
    queue.cancel(toCancel, ...);
  });
});

[chilts]

It's an interesting one with the ObjectID stuff. I can see a number of ways which have been floated here. Let's look at each:

1. Installing MongoDB for the user (as your PR does it @samgurtman) - but I prefer to let them provide their own. I think it's advantageous to allow the user to do since they might be using that specific version for other stuff in their program.
2. Creating your own ObjectID(id) to pass to the queue.cancel() as @aidenkeating says above, which is in contrast to us returning the string IDs everywhere, so there's a certain jarring aspect to that API.
3. Or we could pass the ObjectID function into the constructor at the same time as passing the db - but eww, yuck to that too. :)

Another alternative is to say that I got it wrong by returning a string 'id' in the first place. Maybe this is a datastore independent way, but actually we're not independent since it's all about MongoDB. Perhaps we embrace the IDs (as in, the ObjectIDs) that are returned by queue.add() and actually return the proper ObjectID instead of the string. People using MongoDB already know this concept and it makes more sense.

That also helps fix (in some ways) one of the things about the recent PR regarding the addition of multiple messages to the queue where I notice that we're only returning the first (stringified) ID. Here, we should actually return the array of ObjectIDs so that it actually reflects what has happened.

In conclusion, my thoughts (not suggestions, would love your feedback) are:

1. Return proper ObjectIDs where possible - and therefore it's easier to accept them for Queue.cancel(). I think this is only relevant for Queue.add(). No need to return a string ID but return an ObjectID; no need to install a mongodb package for the user since the ObjectID will be created by the same package they're using; no need to pass in the ObjectID function to the constructor, thereby avoiding all of the problems outlined above.
2. Fix Queue.add() to return a single ObjectID for one payload, return an array for multiple payloads.
3. Queue.cancel() then just uses what has been returned from Queue.add().

How does that sound?

Footnote: And as I look through (unrelated to this issue). (3) sorry about those blah vars, I think that's my fault somewhere, and (4) rename the id() function to newAck() or something better than the confusing name it currently is. :)

[chilts]

Oh crap. @aidenkeating I just said what you said above, except I took a myriad of lines to your two lines. My brain obviously didn't process exactly what you'd said.

So yes, I'd go for the incompatible change rather than the option to the constructor.

[chilts]

And looking at @samgurtman's comment above, I think we're all saying the same thing. Let's use ObjectIDs. :)

Thumbs up?

[samgurtman-zz]

Just to confirm, replace the String returns with ObjectId?

On 16/03/2017 4:34 PM, "Andrew Chilton" notifications@github.com wrote:

And looking at @samgurtman https://github.com/samgurtman's comment above, I think we're all saying the same thing. Let's use ObjectIDs. :)

Thumbs up?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chilts/mongodb-queue/pull/19#issuecomment-286948618, or mute the thread https://github.com/notifications/unsubscribe-auth/AGIKXwLDa1l1tM7blnwHsEmCcnpLquOUks5rmK24gaJpZM4Ma0h5 .

[chilts]

Yes, that's what I'm leaning towards. You can then use it directly in Queue.cancel().

[samgurtman-zz]

I think it should be ready for review now.

[samgurtman-zz]

Woops, let me update the docs unless you want to do that?

[chilts]

Yes please, doc changes would be awesome. :)

I'll take a look at the current changes in the next day or so. Sorry for being quiet recently - I've just come back from a month away. Hopefully will be more responsive now!

[samgurtman-zz]

No worries at all. Sorry I left it a month and a half without any work. We got snowed under with work. I also have another PR coming which I think will make the queue much more usable for complex use cases like Chron Jobs.

[TomKaltz]

@samgurtman Why close?

[samgurtman-zz]

The aims of the PR was better achieved by https://github.com/chilts/mongodb-queue/pull/20 but that was never moved on by maintainers. The open PRs were polluting my github ui, so I closed them due to non activity. I'm fine to reopen https://github.com/chilts/mongodb-queue/pull/20 but I no longer work with mongodb so someone else would have to take over.