Conversation
Test Results - Preflight, Unit23 084 tests ±0 21 225 ✅ ±0 6m 33s ⏱️ -6s Results for commit 7467c04. ± Comparison against base commit 44d1022. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files ± 0 5 suites ±0 1h 35m 8s ⏱️ - 1h 1m 19s Results for commit 7467c04. ± Comparison against base commit 44d1022. This pull request removes 2033 tests.♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 9m 30s ⏱️ - 48m 2s Results for commit 7467c04. ± Comparison against base commit 44d1022. This pull request removes 1636 tests.♻️ This comment has been updated with latest results. |
thrau
left a comment
There was a problem hiding this comment.
Changes look good to me! The callable references were bound to cause issues with persistence at some point, and this solution seems good to me.
Can be merged as is, but have a nit if you want to do an iteration.
| if attr in APPROXIMATE_DYNAMIC_ATTRIBUTES: | ||
| # The approximate_* attributes are calculated on the spot when accessed. | ||
| # We have a @property for each of those which calculates the value. | ||
| value = str(getattr(self, camel_to_snake_case(attr))) |
There was a problem hiding this comment.
i think i would prefer a less magical approach and just have a match/case block. this way we know where the properties are being access:
something like
result: dict[QueueAttributeName, str] = {}
for attr in attribute_names:
try:
getattr(QueueAttributeName, attr)
except AttributeError:
raise InvalidAttributeName(f"Unknown Attribute {attr}.")
value = None
match attr:
case QueueAttributeName.ApproximateNumberOfMessages:
value = self.approx_number_of_messages
case QueueAttributeName.ApproximateNumberOfMessagesDelayed:
value = ...
case _:
value = self.attributes.get(attr)
if value == "False" or value == "True":
result[attr] = value.lower()
elif value is not None:
result[attr] = value
return resultThere was a problem hiding this comment.
Will do, and yes, it is definitely simpler this way. I guess I couldn't resist the magical approach 😃
|
|
||
| # the default maximum message size in SQS | ||
| DEFAULT_MAXIMUM_MESSAGE_SIZE = 1048576 | ||
| APPROXIMATE_DYNAMIC_ATTRIBUTES = [ |
There was a problem hiding this comment.
maybe just call it DYNAMIC_ATTRIBUTES?
Motivation
Our new avro framework cannot serialize the lambda functions for our approximate* attributes. The point of those lambdas was that the approximate* attributes are not like the other typical attrs, but rather calculated freshly every time they are accessed. Since we already need some additional code to call the lambda that is returned, and therefore only access the queue attributes via a helper function
get_queue_attributes, I went one step further and completely removed them from the stored attributes. The necessary logic was moved completely toget_queue_attributes. /cc @giograno please let me know if this fixes the issue at handcloses PNX-585
Changes
Tests
Related