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

Do a basic check of task results before using the files #3227

Merged
merged 10 commits into from Nov 8, 2017

Conversation

Projects
None yet
2 participants
@ericholscher
Member

ericholscher commented Nov 3, 2017

No description provided.

@ericholscher ericholscher requested a review from agjohnson Nov 3, 2017

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Nov 3, 2017

Member

@agjohnson not really happy with this solution, but it at least should solve the 99% case of search getting indexed before the files sync.

Member

ericholscher commented Nov 3, 2017

@agjohnson not really happy with this solution, but it at least should solve the 99% case of search getting indexed before the files sync.

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Nov 3, 2017

Member

Other obvious option is to run it as part of the syncing task, but currently they run on each web, so it would run 4x. Not sure the best middle solution.

Member

ericholscher commented Nov 3, 2017

Other obvious option is to run it as part of the syncing task, but currently they run on each web, so it would run 4x. Not sure the best middle solution.

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Nov 3, 2017

Contributor

This is another case for celery 3+ and task chaining[1]. This is replicating some of the promise structure that a modern celery provides.

I haven't revisited my branch to upgrade celery in a bit, but could put some priority on it.

1: http://docs.celeryproject.org/en/latest/userguide/canvas.html#chains

Contributor

agjohnson commented Nov 3, 2017

This is another case for celery 3+ and task chaining[1]. This is replicating some of the promise structure that a modern celery provides.

I haven't revisited my branch to upgrade celery in a bit, but could put some priority on it.

1: http://docs.celeryproject.org/en/latest/userguide/canvas.html#chains

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Nov 3, 2017

Member

Right, but what do we chain?

Member

ericholscher commented Nov 3, 2017

Right, but what do we chain?

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Nov 3, 2017

Member

We're using the proper version of celery that has linking..

Member

ericholscher commented Nov 3, 2017

We're using the proper version of celery that has linking..

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Nov 3, 2017

Contributor

Ah 3.x, i thought we were 2.x for some reason -- good.

Theoretically we chain on a group, this is what chord() seems to accomplish. I haven't tested this yet though.

Contributor

agjohnson commented Nov 3, 2017

Ah 3.x, i thought we were 2.x for some reason -- good.

Theoretically we chain on a group, this is what chord() seems to accomplish. I haven't tested this yet though.

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Nov 3, 2017

Contributor

A quick test shows an example of what we're ultimately looking for, though I'm calling .get here, which still ties up the builder waiting for replies, something we don't want:
https://gist.github.com/agjohnson/19666953a7673cd24630b7091f37b8ca

Pushing this all on to celery makes more sense as if a web queue is offline or backed up, we won't hit the 5sec * 4 timeouts and won't hit deadlocks. We'll want to try to make sure the linked task/chord has a timeout though, as extended backup on the webs could cause a large number of chord unlocks on the celery workers.

I get a loop of unlocking the chord on the example above locally though. I'm not sure if it's related to the version, but upgrading 3.1.23 -> 4.1.x resolved the issue. Things worked on 3.1.23, there just seemed to be unwarranted unlock tasks executed for some reason.

Contributor

agjohnson commented Nov 3, 2017

A quick test shows an example of what we're ultimately looking for, though I'm calling .get here, which still ties up the builder waiting for replies, something we don't want:
https://gist.github.com/agjohnson/19666953a7673cd24630b7091f37b8ca

Pushing this all on to celery makes more sense as if a web queue is offline or backed up, we won't hit the 5sec * 4 timeouts and won't hit deadlocks. We'll want to try to make sure the linked task/chord has a timeout though, as extended backup on the webs could cause a large number of chord unlocks on the celery workers.

I get a loop of unlocking the chord on the example above locally though. I'm not sure if it's related to the version, but upgrading 3.1.23 -> 4.1.x resolved the issue. Things worked on 3.1.23, there just seemed to be unwarranted unlock tasks executed for some reason.

@ericholscher

This comment has been minimized.

Show comment
Hide comment
@ericholscher

ericholscher Nov 7, 2017

Member

@agjohnson should be ready for another review.

Member

ericholscher commented Nov 7, 2017

@agjohnson should be ready for another review.

@ericholscher ericholscher merged commit c9f385b into master Nov 8, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@agjohnson agjohnson deleted the search-race-condition branch Nov 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment