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 DocumentDB support #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

jaywink
Copy link
Member

@jaywink jaywink commented Aug 8, 2022

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.

$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
Copy link
Member

@Half-Shot Half-Shot left a 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>
Copy link
Member

Choose a reason for hiding this comment

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

Job Deleted successfully, surely?

Comment on lines 68 to 100
},
{
$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] }],
Copy link
Member

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 = [
Copy link
Member

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.

.then((result) => {
return result;
})
.catch((error) => {
Copy link
Member

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 😮‍💨

Copy link
Member Author

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.

Copy link
Member Author

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.
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.

3 participants