Fix local storage and content removal #125
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Would this clean up files already not in the database, or would I have to manually painstakingly clean up myself after this rolls out? I don't want to set a temporary super short retention time so it'll clean up files already "gone" according to Synapse (like when accounts get deleted and such) |
There was a problem hiding this comment.
Thanks for your efforts on the PR, and apologies for the slow review response.
I've had a brief look at this PR, but there are several red flags that signal "sloppy AI" to me. Please review the comments I've left and clean up the PR a bit before the next round of review.
In addition, when making multiple distinct changes ("Fixed file handling logic in store_file method", "Added retention features"), please open separate PRs for each to make review simpler.
| s3_config = config.get("config", {}) | ||
| if not s3_config: | ||
| logger.warning("No config block found, falling back to root-level settings") | ||
| self.bucket = config.get("bucket") | ||
| self.api_kwargs = { | ||
| "region_name": config.get("region_name"), | ||
| "endpoint_url": config.get("endpoint_url"), | ||
| "aws_access_key_id": config.get("access_key_id"), | ||
| "aws_secret_access_key": config.get("secret_access_key"), | ||
| } | ||
| self.prefix = config.get("prefix", "") | ||
| self.extra_args = config.get("extra_args", {}) | ||
| else: | ||
| self.bucket = s3_config.get("bucket") | ||
| self.api_kwargs = { | ||
| "region_name": s3_config.get("region_name"), | ||
| "endpoint_url": s3_config.get("endpoint_url"), | ||
| "aws_access_key_id": s3_config.get("access_key_id"), | ||
| "aws_secret_access_key": s3_config.get("secret_access_key"), | ||
| } | ||
| self.prefix = s3_config.get("prefix", "") | ||
| self.extra_args = s3_config.get("extra_args", {}) |
There was a problem hiding this comment.
config contains the contents of the config key. It won't contain a config key itself, so this if isn't necessary.
| """Get or create an S3 client.""" | ||
| if self._s3_client is None: | ||
| with self._s3_client_lock: | ||
| if self._s3_client is None: | ||
| self._s3_client = boto3.client("s3", **self.api_kwargs) | ||
| return self._s3_client |
There was a problem hiding this comment.
Why have you removed the comments from this method?
| # all the data has been written (or there has been a fatal error). | ||
| self.deferred = defer.Deferred() | ||
|
|
||
There was a problem hiding this comment.
There are many instances of whitespace being added unnecessarily. Please remove them.
| from setuptools import setup | ||
|
|
||
| __version__ = "1.5.0" | ||
| __version__ = "1.5.1" |
There was a problem hiding this comment.
Please don't update the version in a feature PR. We'll do that during a release.
| self.local_media_lifetime = parse_duration(media_retention.get("local_media_lifetime")) | ||
| self.remote_media_lifetime = parse_duration(media_retention.get("remote_media_lifetime")) |
There was a problem hiding this comment.
parse_duration will raise an exception if media_retention.get returns None.
| # Triggered by responder when more data has been requested (or | ||
| # stop_event has been triggered) | ||
| self.wakeup_event = threading.Event() | ||
| # Trigered by responder when we should abort the download. | ||
| self.stop_event = threading.Event() | ||
|
|
||
| # The consumer we're registered to | ||
| self.consumer = None | ||
|
|
||
| # The deferred returned by write_to_consumer, which should resolve when | ||
| # all the data has been written (or there has been a fatal error). |
There was a problem hiding this comment.
Again, why are you removing only comments?
4 participants
Closes #121
Closes #115
Closes #96
Closes #28
Changes Made
store_filemethod:store_local: truewith retention - files are kept locally for the specified durationImproved configuration handling:
media_retentionsettings outside theconfigblockAdded retention features:
remote_media_lifetimefunctionality to delete files from S3 after specified durationremote_media_lifetimeis not setreactor.callLaterfor both local and remote filesEnhanced logging:
Documentation
remote_media_lifetimefeature