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 sub-table name for dynamodb storage #123

Closed

Conversation

kylebarron
Copy link
Member

Closes #116

  • What should be the name of the subkey? subtable_name doesn't sound great
  • This defines the canonical path as dynamodb://{region}/{table_name}:{subtable_name}. A colon is not a valid DynamoDB table identifier
  • I don't know why Mypy is unhappy with cogeo_mosaic/backends/dynamodb.py:64: error: Module has no attribute "matches_re"

It would probably also be good to have

  • a delete method, so that you can delete one of the subtables. This would read the metadata for a single subkey and then iterate over its quadkeys, calling a delete operation on each one
  • a key to list all subkeys that exist in the table. Use something like -2 (?) (since we already use -1 for the mosaic metadata), and have it contain a list of strings that are the subkeys that exist in the table. This would need to stay updated with the CRUD operations.
  • Would we want to prevent a full overwrite of the table? Currently one of the methods will delete and create a new table if overwrite=True.

@kylebarron kylebarron marked this pull request as draft October 15, 2020 03:36
self.table_name = table_id[0]
if len(table_id) > 1:
self.subtable_name = table_id[1]

self.region = parsed.netloc or self.region
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about making region mandatory in the path?

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 think it would be fine to make that explicit.

@vincentsarago
Copy link
Member

@kylebarron thanks for starting this!

With help from @leothomas I've created a new PR to handler this #127

validator=attr.validators.in_(AWS_REGIONS),
)
table_name: str = attr.ib(
init=False, validator=attr.validators.matches_re(r"^[a-zA-Z0-9\_\-\.]+$")
Copy link
Member

Choose a reason for hiding this comment

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

@kylebarron validators only run on init, so this won't work

Copy link
Member

Choose a reason for hiding this comment

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

@kylebarron because the table_name is set during the __post_init the validation is not happening! you'll need to re-run the validation at the end of the __post_init__

table_name: str = attr.ib(init=False)
region: str = attr.ib(
default=os.getenv("AWS_REGION", "us-east-1"),
validator=attr.validators.in_(AWS_REGIONS),
Copy link
Member

Choose a reason for hiding this comment

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

I prefer not having a list to validate to because everytime AWS will add a new region we will need a new release

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.

Multiple mosaics within a single DynamoDB table
2 participants