-
Notifications
You must be signed in to change notification settings - Fork 1
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 DocumentDB support #2
base: master
Are you sure you want to change the base?
Conversation
$facet is not available in AWS Document DB and throws an error. Created an error check that if it fails with the standard query to try the AWS query to create the pages and filters.
Fixed the total page count
removed line uses for testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the fallback logic here is flawed.
<div v-if="deletec" class="alert alert-success popupmessage">Job Deleted successfull</div> | ||
<div v-else-if="requeuec" class="alert alert-success popupmessage">Job Requeue successfull</div> | ||
<div v-else-if="createc" class="alert alert-success popupmessage">Job Created successfull</div> | ||
<div v-if="deletec" class="alert alert-success popupmessage">Job Deleted successful</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Job Deleted successfully, surely?
lib/controllers/agendash.js
Outdated
}, | ||
{ | ||
$project: { | ||
job: "$$ROOT", | ||
_id: "$$ROOT._id", | ||
running: { | ||
$and: ["$lastRunAt", { $gt: ["$lastRunAt", "$lastFinishedAt"] }], | ||
}, | ||
scheduled: { | ||
$and: ["$nextRunAt", { $gte: ["$nextRunAt", new Date()] }], | ||
}, | ||
queued: { | ||
$and: [ | ||
"$nextRunAt", | ||
{ $gte: [new Date(), "$nextRunAt"] }, | ||
{ $gte: ["$nextRunAt", "$lastFinishedAt"] }, | ||
], | ||
}, | ||
completed: { | ||
$and: [ | ||
"$lastFinishedAt", | ||
{ $gt: ["$lastFinishedAt", "$failedAt"] }, | ||
], | ||
}, | ||
failed: { | ||
$and: [ | ||
"$lastFinishedAt", | ||
"$failedAt", | ||
{ $eq: ["$lastFinishedAt", "$failedAt"] }, | ||
], | ||
}, | ||
repeating: { | ||
$and: ["$repeatInterval", { $ne: ["$repeatInterval", null] }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whitespace changes make this difficult to review
} | ||
} | ||
] | ||
const awsDocumentDBQuery = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explain the limitation with this query, for some reason we're trying to run a query, failing and then trying to run another query that's docDB compatible.
lib/controllers/agendash.js
Outdated
.then((result) => { | ||
return result; | ||
}) | ||
.catch((error) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gross, it's an extra round trip to AWS every time we run this query, and I'm not sure amazon will thank us for that. Can we just make this a configuration option (or somehow smartly detect the version of Mongo, rather than this awful thing?)
We're not even looking at the error response to detect why it failed 😮💨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping to avoid us maintaining our own code until upstream implements this properly. This works and it's only doing this query every so and so seconds. I hardly think AWS will notice.
We can reimplement this patch but I'm going to postpone it after staging migration, if ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify the reason for not spending engineering time now: when upstream implements documentdb support, we're going to want to use that implementation anyway, not whatever we came up with. As such I'd avoid making anything fancy at this stage. Given we only need documentdb support, I could simplify the pr - but that carries the issue if the upstream patch gets new commits, we'll be in a merge conflict against that.
Make a proper patch where it is configurable later.
The DocumentDB query barfs on large data sets. This is a temporary fix until the documentdb query is properly fixed.
Patch from agenda#208 - merging in here in our fork given it is not yet merged by upstream. Have checked the code and tested it, seems to work so far.