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

Local dynamodb doesn't support DescribeTimeToLive #117

Closed
lillesand opened this issue Apr 16, 2018 · 12 comments
Closed

Local dynamodb doesn't support DescribeTimeToLive #117

lillesand opened this issue Apr 16, 2018 · 12 comments
Assignees

Comments

@lillesand
Copy link

The local dynamodb doesn't support DescribeTimeToLive. With 4fb66f0 TTL has been moved from a optional Meta-setting to something that is done for every model. This effectually breaks support of local dynamodb from 2.1.0.

I see this has been fixed in tests with 7720d11. Maybe a similar approach could be used as a fallback in session.py?

One could argue that this is a problem with local dynamodb rather than Bloop itself, but I think we'll have a better chance of having it fixed in Bloop 😊.

@numberoverzero
Copy link
Owner

numberoverzero commented Apr 17, 2018

Thanks for opening an issue, I should have realized this would be a problem given the integ test patch. I'm sorry if this broke any local testing.

Short term fix -- revert to 2.0.1 if you're not using the Server-Side Encryption feature added in 2.1.0

Options

(in no particular order)

The integ test stub can't be used generally in the SessionWrapper class because it always returns an empty response, and engine.bind will fail if your model declares a ttl. This only works in the integ test because none of those models specify a ttl.

One fix would involve only calling DescribeTimeToLive when the model specifies a ttl value (model.Meta.ttl is not None) during SessionWrapper.describe_table but I don't like this fix because at the low level, describe_table should make the same outbound calls regardless of the model being described. This abstraction (SessionWrapper) exists to smooth over the various API calls that need to be made to fully represent a table, and to instead selectively make calls and omit some information based on inputs would reduce the utility.

Another fix is to recommend (and potentially provide documentation for) setting up https://github.com/localstack/localstack which seems to keep up to date with new api features and is going to be a bit simpler if you use more AWS services than just DynamoDB (here's the issue where they added TTL support localstack/localstack#599). While this is a significant refactor to ask bloop users to take on, I expect DynamoDBLocal will continue to fall behind Dynamo's newest features and this problem will be more visible.

An in-between option is to modify the dynamodb client that you pass to bloop.Engine (from the docs)and customize the behavior yourself; this lets you either copy the integ test behavior directly (always return no TTL information) or to match the table name against a list and return the correct mocked TTL info. I could add this to the bloop patterns section so that it's easier to grab in one go. I don't love documentation fixes in general but since users will probably need to look this section up anyway it could be a decent compromise.

2.2+

I don't expect to fix this in 2.1.0 but I also don't expect Bloop to have any releases until 2.2.0. As mentioned in the second option DynamoDBLocal will probably continue to fall behind the DynamoDB feature set. Given this divergence will only worsen I'm leaning towards the third option: provide documentation for the necessary shims when connecting to DynamoDBLocal in a single code sample that users can copy-paste. It might look like this (untested):

import boto3
import bloop


class PatchedDynamoDBClient:
    def __init__(self, real_client):
        self.__client = real_client

    def describe_time_to_live(self, TableName, **_):
        return {"TimeToLiveDescription": {"TimeToLiveStatus": "DISABLED"}}

    def describe_continuous_backups(self, TableName, **_):
        return {"ContinuousBackupsDescription": {"ContinuousBackupsStatus": "DISABLED"}}

    # TODO override any other methods that DynamoDBLocal doesn't provide

    def __getattr__(self, name):
        # use the original client for everything else
        return getattr(self.__client, name)


def patched_local_engine(endpoint="http://127.0.0.1:8000", **engine_kwargs):
    dynamodb = boto3.client("dynamodb", endpoint_url=endpoint)
    dynamodbstreams = boto3.client("dynamodbstreams", endpoint_url=endpoint)
    return bloop.Engine(
        dynamodb=PatchedDynamoDBClient(dynamodb),
        dynamodbstreams=dynamodbstreams,
        **engine_kwargs
    )


engine = patched_local_engine(table_name_template="local-{table_name}")

@lillesand
Copy link
Author

Thanks for a very thorough response, it's greatly appreciated.

I guess there are two use cases we ideally should cover:

  1. Use in automated testing.
  2. Local development. I don't know how common this use case is for dynamodb, but I at least like to be able to develop while offline.

If we except DynamoDBLocal to keep falling behind I definitely agree that it sounds like a bad idea to try to maintain support in Bloop.

I'm not very familiar with localstack, but if it supports both of the above use cases it does sound like the best forward. As long as Bloop already works with it (… and I don't see why it shouldn't!), it basically would require no work except documentation. And that's nice 😊.

@numberoverzero
Copy link
Owner

I'm going to try migrating the integ tests to localstack, and if it's easy enough then I'll update the docs to recommend it over DynamoDBLocal as well. This would also avoid the sha checksum for the s3 download (which is useful but noisy when the artifact changes).

@numberoverzero
Copy link
Owner

Not the best timing on my part, pip install localstack is failing on localstack/localstack#721 or a related issue. The packaging structure is making it a little harder to debug the installation failure which is a valuable data point on its own; I'd want to pin to an exact version if we're going to use this library.

@numberoverzero
Copy link
Owner

numberoverzero commented Apr 18, 2018

Installation is working again but I noticed some strange traffic and found that localstack emits usage data and it's occurring on startup, teardown, and table create/delete. I'm not ready to have that running in my integ tests, much less recommend it in the docs.

I'll open an issue to see if they can add an option to disable collection, but I'm a little skeptical given there's already an EULA. If there's no way to disable it then we're back to documenting DynamoDBLocal usage and the necessary shims (or writing a new DynamoDBLocal package but that's overkill).

@numberoverzero
Copy link
Owner

Cut a new issue localstack/localstack#728 to see if disabling analytics is possible.

@lillesand
Copy link
Author

This is great work, @numberoverzero. Thanks a lot for taking this so seriously and the great work you do on Bloop.

I fully agree on your decided way forward, for what it's worth.

@trondhindenes
Copy link

just for ref, in the above patch-example, code should be:

def patched_local_engine(endpoint="http://127.0.0.1:8000", **engine_kwargs):
    dynamodb = boto3.client("dynamodb", endpoint_url=endpoint)

@numberoverzero
Copy link
Owner

Thanks for the fix, updated the comment.

I poked localstack/localstack#728 again but it looks like there may not be an answer. That probably means the documentation needs to be updated in bloop to include an example of patching out those methods in DynamoDB.

Also learned while doing some testing that the DynamoDBLocal sha changed, which means builds will start failing. I'll try to get to that soon.

@numberoverzero
Copy link
Owner

Hey @lillesand and @trondhindenes,

With the release of 2.2 (#123) I believe this can be closed.
Here are the new and updated docs:

If you think anything's missing or could be improved feel free to comment or re-open. Thanks for your feedback!

@lillesand
Copy link
Author

Looks great, thanks! 👏

@trondhindenes
Copy link

good stuff! I'm fine with closing this as it's solved thru updated docs.

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

No branches or pull requests

3 participants