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

Ability to capture asynchronous responses #1061

Merged
merged 8 commits into from Aug 21, 2017

Conversation

Projects
None yet
4 participants
@scoates
Collaborator

scoates commented Aug 16, 2017

Description

Implements a mechanism (capture_response) to optionally capture the result of asynchronous tasks to DynamoDB.

Example (see the README for more):

@app.route('/payload')
def payload():
    delay = request.args.get('delay', 60)
    x = longrunner(delay)
    return "Got response_id: {}".format(x.response_id)

@task(capture_response=True)
def longrunner(delay):
    sleep(float(delay))
    return {'MESSAGE': "It took {} seconds to generate this.".format(delay)}

Updates settings to allow specification of table name.

Updates README to document this feature, including an example app.

GitHub Issues

Closes #1060

Other

I think this can be considered for merge because the functionality is useful right away, but it's missing a few things, longer term:

  • DynamoDB table creation uses static values (5) for read + write provisioning, which currently costs "$2.91 / month" according to the console. Would be really nice to make this autoscale, but that's much more complicated.
  • I did not add a test for this. It sounds extremely difficult to write a test that implements this without actually deploying an app during the test suite run.
  • Timeout is still limited to the lambda limit (timeout_seconds in zappa settings), so there's a hard limit of 300s for these. The DynamoDB records have a TTL of ~600 (+lag) seconds; this is not configurable at this time.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 16, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.525% when pulling dbcf28b on scoates:async_response into 57ec5ff on Miserlou:master.

coveralls commented Aug 16, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.525% when pulling dbcf28b on scoates:async_response into 57ec5ff on Miserlou:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 16, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.525% when pulling dbcf28b on scoates:async_response into 57ec5ff on Miserlou:master.

coveralls commented Aug 16, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.525% when pulling dbcf28b on scoates:async_response into 57ec5ff on Miserlou:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 16, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.525% when pulling dbcf28b on scoates:async_response into 57ec5ff on Miserlou:master.

coveralls commented Aug 16, 2017

Coverage Status

Coverage decreased (-0.7%) to 73.525% when pulling dbcf28b on scoates:async_response into 57ec5ff on Miserlou:master.

@bcongdon

This comment has been minimized.

Show comment
Hide comment
@bcongdon

bcongdon Aug 16, 2017

Contributor

Great feature!

I'd place a high emphasis on being able to configure the read/write capacity provisioning, though. Based on the experience I've had with Dynamo, 5 read/write is probably overkill for toy examples, and not enough for something that gets a modest amount of load.

Contributor

bcongdon commented Aug 16, 2017

Great feature!

I'd place a high emphasis on being able to configure the read/write capacity provisioning, though. Based on the experience I've had with Dynamo, 5 read/write is probably overkill for toy examples, and not enough for something that gets a modest amount of load.

@scoates

This comment has been minimized.

Show comment
Hide comment
@scoates

scoates Aug 16, 2017

Collaborator

Agreed. I'll work on this.

Collaborator

scoates commented Aug 16, 2017

Agreed. I'll work on this.

make read and write capacities configurable. have Zappa warn if the c…
…apacities don't match configured values
@scoates

This comment has been minimized.

Show comment
Hide comment
@scoates

scoates Aug 17, 2017

Collaborator

Does the change in 03314e1 work for you, @bcongdon?

Collaborator

scoates commented Aug 17, 2017

Does the change in 03314e1 work for you, @bcongdon?

@bcongdon

This comment has been minimized.

Show comment
Hide comment
@bcongdon

bcongdon Aug 17, 2017

Contributor

Yeah, awesome!

Contributor

bcongdon commented Aug 17, 2017

Yeah, awesome!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 17, 2017

Coverage Status

Coverage decreased (-0.8%) to 73.468% when pulling 03314e1 on scoates:async_response into 57ec5ff on Miserlou:master.

coveralls commented Aug 17, 2017

Coverage Status

Coverage decreased (-0.8%) to 73.468% when pulling 03314e1 on scoates:async_response into 57ec5ff on Miserlou:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 17, 2017

Coverage Status

Coverage decreased (-0.8%) to 73.493% when pulling 6e571dc on scoates:async_response into 57ec5ff on Miserlou:master.

coveralls commented Aug 17, 2017

Coverage Status

Coverage decreased (-0.8%) to 73.493% when pulling 6e571dc on scoates:async_response into 57ec5ff on Miserlou:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 17, 2017

Coverage Status

Coverage decreased (-0.8%) to 73.493% when pulling 5901b85 on scoates:async_response into 57ec5ff on Miserlou:master.

coveralls commented Aug 17, 2017

Coverage Status

Coverage decreased (-0.8%) to 73.493% when pulling 5901b85 on scoates:async_response into 57ec5ff on Miserlou:master.

@scoates

This comment has been minimized.

Show comment
Hide comment
@scoates

scoates Aug 17, 2017

Collaborator

Ok, @Miserlou, I'm done fiddling for now. Merge at will.

Collaborator

scoates commented Aug 17, 2017

Ok, @Miserlou, I'm done fiddling for now. Merge at will.

@Miserlou Miserlou merged commit f353676 into Miserlou:master Aug 21, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.8%) to 73.493%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@scoates scoates deleted the scoates:async_response branch Aug 22, 2017

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