FIX: Do not attempt S3 ACL call if secure status did not change #24785
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When rebaking and in various other places for posts, we run through the uploads and call `update_secure_status` on each of them. However, if the secure status didn't change, we were still calling S3 to change the ACL, which would have been a noop in many cases and takes ~1 second per call, slowing things down a lot. Also, we didn't account for the s3_acls_enabled site setting being false here, and in the specs doing an assertion that `Discourse.store.update_ACL` is not called doesn't work; `Discourse.store` isn't a singleton, it re-initializes `FileStore::S3Store.new` every single time.
tgxworld
approved these changes
Dec 8, 2023
tgxworld
reviewed
Dec 8, 2023
martin-brennan
deleted the
issue/do-not-change-acl-if-secure-not-changed
branch
tgxworld
pushed a commit
to tgxworld/discourse
that referenced
this pull request
Dec 8, 2023
…ourse#24785) When rebaking and in various other places for posts, we run through the uploads and call `update_secure_status` on each of them. However, if the secure status didn't change, we were still calling S3 to change the ACL, which would have been a noop in many cases and takes ~1 second per call, slowing things down a lot. Also, we didn't account for the s3_acls_enabled site setting being false here, and in the specs doing an assertion that `Discourse.store.update_ACL` is not called doesn't work; `Discourse.store` isn't a singleton, it re-initializes `FileStore::S3Store.new` every single time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When rebaking and in various other places for posts, we
run through the uploads and call
update_secure_statusoneach of them.
However, if the secure status didn't change, we were still
calling S3 to change the ACL, which would have been a noop
in many cases and takes ~1 second per call, slowing things
down a lot.
Also, we didn't account for the s3_acls_enabled site setting
being false here, and in the specs doing an assertion
that
Discourse.store.update_ACLis not called doesn'twork;
Discourse.storeisn't a singleton, it re-initializesFileStore::S3Store.newevery single time.