feat: Make dynamo table tagging opt-in #5438
Conversation
Signed-off-by: Rob Howley <rhowley@seatgeek.com>
Signed-off-by: Rob Howley <rhowley@seatgeek.com>
robhowley
force-pushed
the
rh-skip-tagging
branch
from
8a96b86 to
acbaad4
Compare
robhowley
changed the title
Signed-off-by: Rob Howley <rhowley@seatgeek.com>
robhowley
changed the title
| pass | ||
|
|
||
|
|
||
| class _DynamoTableManager: |
There was a problem hiding this comment.
the online store class has become a cluttered god class, move the dynamo table creation/destruction to a helper class
| @retry( | ||
| wait=wait_exponential(multiplier=1, max=4), | ||
| retry=retry_if_exception_type(RetryableBotoError), | ||
| stop=stop_after_attempt(3), | ||
| reraise=True, | ||
| ) |
There was a problem hiding this comment.
tagging a lot of dynamo tables can lead to api rate limiting errors, retry with backoff
| tag_aws_resources: StrictBool = False | ||
| """Add the feature-view tags to the underlying AWS dynamodb tables""" |
There was a problem hiding this comment.
this feature isnt strictly necessary (though a good idea!) and results in a a few extra aws api calls per feature view, so make it opt in.
|
|
||
| class _DynamoTableManager: | ||
| def __init__( | ||
| self, dynamodb_resource, config: RepoConfig, feature_view: FeatureView |
There was a problem hiding this comment.
| self, dynamodb_resource, config: RepoConfig, feature_view: FeatureView | |
| self, dynamodb_resource, config: RepoConfig, table: FeatureView |
I would rather keep the parameter name in the context of Database that is table but accept FeatureView object as is suggests.
| self, dynamodb_resource, config: RepoConfig, feature_view: FeatureView | ||
| ): | ||
| self.config = config | ||
| self.feature_view = feature_view |
There was a problem hiding this comment.
| self.feature_view = feature_view | |
| self.feature_view = table |
| AttributeDefinitions=[ | ||
| {"AttributeName": "entity_id", "AttributeType": "S"} | ||
| ], | ||
| BillingMode="PAY_PER_REQUEST", |
There was a problem hiding this comment.
I think billing mode should be taken for parameters and not hardcoded ?
There was a problem hiding this comment.
- @robhowley can you set this as something configurable with the default being
PAY_PER_REQUEST?
| BillingMode="PAY_PER_REQUEST", | ||
| **kwargs, | ||
| ) | ||
| do_tag_update = False |
There was a problem hiding this comment.
Why are we setting it to False ? Good to annotate ?
What this PR does / why we need it:
tag_aws_resources: bool = Falsesample error one can get on apply
Which issue(s) this PR fixes:
Misc