Conversation
sophia-bq
force-pushed
the
refactor/services-container
branch
2 times, most recently
from
df6922a to
e5f475b
Compare
aws_advanced_python_wrapper/resources/aws_advanced_python_wrapper_messages.properties
Outdated
Show resolved
Hide resolved
sophia-bq
force-pushed
the
refactor/services-container
branch
from
e5f475b to
96742b9
Compare
| _event_publisher: Optional[BatchingEventPublisher] = None | ||
| _storage_service: Optional[StorageService] = None | ||
| _monitor_service: Optional[MonitorService] = None |
There was a problem hiding this comment.
Curious, do u think we need to lazily initialize these? Or would it be better to just initialize these since all connections will pretty much use them.
I like what you did here cause I feel like it's a lot more pythonic than having dependency injection everywhere, the only code smell I see is global keyword. It's not inherently bad, but would make the code cleaner (and possibly be able to remove the lock), if we just initialize these. I'm fine either way though.
There was a problem hiding this comment.
We need to be able to re-initialize them - I've gone with a services container style approach that should have no global keyword and initializes them at the start. It's a little less pythonic but fits our use case
| for key, cache_item in container.cache.items(): | ||
| if cache_item.item is monitor: | ||
| container.cache._cdict.remove(key) |
There was a problem hiding this comment.
Is there a way for us to remove this without doing O(n)?
There was a problem hiding this comment.
This particular one is because of how efm does aliases as keys for the monitors - it makes it so that we can't access by key unless we do a reverse dict
3 participants
Description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.