Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Purge variants with their blobs #31319

Merged
merged 1 commit into from Dec 3, 2017

Conversation

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Dec 2, 2017

Fixes #31290.

@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:ast-variant-purging branch 2 times, most recently Dec 3, 2017
@georgeclaghorn georgeclaghorn force-pushed the georgeclaghorn:ast-variant-purging branch to 11af5fc Dec 3, 2017
@georgeclaghorn georgeclaghorn merged commit 8c5a7fb into rails:master Dec 3, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@georgeclaghorn georgeclaghorn deleted the georgeclaghorn:ast-variant-purging branch Dec 3, 2017
Copy link
Member

@kaspth kaspth left a comment

Looks great! Just had some follow up questions out of curiosity mostly 😊

@@ -270,7 +270,8 @@ def analyzed?
# deleted as well or you will essentially have a dead reference. It's recommended to use the +#purge+ and +#purge_later+
# methods in most circumstances.
def delete
service.delete key
service.delete(key)
service.delete_prefixed("variants/#{key}/") if image?

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Why do we only want this for image? variants? Isn't it a little sneaky that the delete behavior is different depending on the blob type?

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Dec 3, 2017
Author Member

Non-image blobs don’t have variants. Skipping this saves an unnecessary API call.

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Ah, right 👍

@@ -51,16 +51,32 @@ def delete(key)
end
end

def delete_prefixed(prefix)
instrument :delete_all, prefix: prefix do

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Fixed the instrumentation name to match the others in: 7609ca0

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Dec 3, 2017
Author Member

Gah, thanks!

instrument :delete_all, prefix: prefix do
marker = nil

loop do

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Could this potentially block forever? Or does list_blobs time out? Does it raise an exception when it does?

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Dec 3, 2017
Author Member

We can provide the :timeout option to list_blobs if we wish, but it returns a limited number of blobs for each call (5,000 by default), so I don’t see why it’d block indefinitely.

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

👍, the loop do just had me a bit worried 😊

@@ -48,16 +48,24 @@ def delete(key)
end
end

def delete_prefixed(prefix)
instrument :delete_prefixed, prefix: prefix do
Dir.glob(path_for("#{prefix}*")).each do |path|

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Dir.glob can potentially return . and ... Should we do something specific here to filter them out? Or are they always filtered out via the prefix? (What happens if there's a blank prefix?)

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Dec 3, 2017
Author Member

Dir.glob can potentially return . and ...

I was under the impression it would only do so if we set the FNM_DOTMATCH flag:

irb(main):006:0> Dir.glob("/Users/george/Desktop/*")
=> []
irb(main):007:0> Dir.glob("/Users/george/Desktop/*", File::Constants::FNM_DOTMATCH)
=> ["/Users/george/Desktop/.", "/Users/george/Desktop/..", "/Users/george/Desktop/.DS_Store", "/Users/george/Desktop/.localized"]

Is that platform-dependent?

Or are they always filtered out via the prefix?

In Active Storage’s own usage, yes.

What happens if there's a blank prefix?

All objects are deleted. That seems correct, though perhaps not desirable.

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Is that platform-dependent?

Not sure. It could be Ruby version dependent. I definitely remember seeing it in our app on Ruby 2.2.

All objects are deleted. That seems correct, though perhaps not desirable.

I meant to say: if . and .. are included and the prefix is blank would it delete the current and parent directories?

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Dec 3, 2017
Author Member

In Active Storage’s own usage, yes.

I’m wrong. Active Storage’s usage of delete_prefixed would theoretically encounter this.

Not sure. It could be Ruby version dependent. I definitely remember seeing it in our app on Ruby 2.2.

I’m not seeing it on Ruby 2.2:

irb(main):002:0> RUBY_VERSION
=> "2.2.8"
irb(main):003:0> Dir.glob("/Users/george/Desktop/*")
=> []
irb(main):004:0> Dir.glob("/Users/george/Desktop/*", File::Constants::FNM_DOTMATCH)
=> ["/Users/george/Desktop/.", "/Users/george/Desktop/..", "/Users/george/Desktop/.DS_Store", "/Users/george/Desktop/.localized"]

I meant to say: if . and .. are included and the prefix is blank would it delete the current and parent directories?

If they’re returned by Dir.glob, yes.

This comment has been minimized.

@kaspth

kaspth Dec 16, 2017
Member

Ah, I was thinking of Dir.entries. No worries here then 👍

@@ -47,16 +47,22 @@ def delete(key)
end
end

def delete_prefixed(prefix)
instrument :delete_prefixed, prefix: prefix do
bucket.files(prefix: prefix).all(&:delete)

This comment has been minimized.

@kaspth

kaspth Dec 3, 2017
Member

Feels strange that the gcs API object uses all and not each 😄

@victorbueno
Copy link

@victorbueno victorbueno commented Dec 4, 2017

@georgeclaghorn Thank you for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.