vsivsi / meteor-job-collection

A persistent and reactive job queue for Meteor, supporting distributed workers that can run anywhere.
https://atmospherejs.com/vsivsi/job-collection
Other
388 stars 68 forks source link

Missing index? #251

Closed sdarnell closed 7 years ago

sdarnell commented 7 years ago

The wtJobs collection has an index on type and status, but not status alone. _promote_jobs uses a query that just checks status and expiresAfter resulting in mongodb doing a COLLSCAN.

If the jobs collection has accumulated a number of failed/completed entries, this is potentially inefficient.

Is there any way that the type could be included in the query, or does it make sense to add an index just on the status column?

vsivsi commented 7 years ago

Hi, thanks for pointing this out. When I first wrote this package, I intentionally defined no default indexes... Under the theory that job collections would typically not be huge (and so the default case was "good enough"...) For applications where that did not hold, the developers would be savvy enough to do their own benchmarking and decide which indexes made the most sense for their situation and define them on their own.

Then at some point, somebody wore down my resolve and convinced me that I should just define reasonable indexes, and so I did. There are two problems with that:

1) indexes are not free. They potentially slow down every document update. So adding indexes will not automatically be a net positive in every case.

2) the "right" indexes depend on the queries that are being run, and as a system evolves, the set of frequent queries can change (or be added to). But in the code, where the indexes are defined is separated from where the queries are defined and run... So you get situations like you have identified here, when one changes and the other does not.

And these are the two reasons why I wanted to leave out defining default indexes to begin with!

If the goal is to be "optimal" from the perspective of maintaining a huge Job Collection, then there probably need to be two additional compound indexes:

@_ensureIndex { status : 1, after: 1 }         # For jobReady()
@_ensureIndex { status : 1, expiresAfter: 1 }  # For _promote_jobs()

Or maybe this is getting too specific and the best thing to do is have individual indexes:

@_ensureIndex { type : 1 }
@_ensureIndex { status : 1 }
@_ensureIndex { after: 1 }
@_ensureIndex { expiresAfter: 1 }
@_ensureIndex { priority : 1, retryUntil : 1 }

It's kind of hard to know without benchmarking a specific application under realistic workloads...

Or maybe the right thing to do is remove the default indexes once again and just punt this back to the developers to do their own benchmarking and make the correct decisions for their own apps.

Ugh.

vsivsi commented 7 years ago

Relevant history: https://github.com/vsivsi/meteor-job/issues/3 https://github.com/vsivsi/meteor-job-collection/issues/57 https://github.com/vsivsi/meteor-job-collection/issues/122

sdarnell commented 7 years ago

Ah, I see this is a tricky subject - I'm coming to this afresh with little previous knowledge of JC, and investigating some performance issues of our app. As a new user to the package I tend to agree with chmac's comments from https://github.com/vsivsi/meteor-job-collection/issues/57#issuecomment-70945432

It seems unreasonable for a user of this package to have to know how it operates internally, appreciate all the queries it makes etc. Yet the package is clearly quite configurable and can be used in different ways, with different data usage patterns.

In our case we have added an index on 'status', and are now clearing out jobs more than we were before. Maybe the status quo (default key indexes plus readme guidance) is the best option. Thanks for the consideration.