Cache fixes #209

shaiguitar · 2017-05-09T14:33:10Z

Not important to merge/release at this point, as there might be some more iterations to go through. But logging this here so I don't forget.

There might be some more fixes of these sorts of things over time as usage increases.

plribeiro3000 · 2017-05-09T15:05:45Z

lib/fog/core/cache.rb

+          model_klass.new(load_cache(path)[:attrs])
+        else
+          Fog::Logger.warning("Found corrupt items in the cache: #{path}. Expire all & refresh cache soon.\n\nData:#{File.read(path)}")
+        end


I wonder where this check should be done here or in the load_cache method instead.
This one is big enough already.

Sounds reasonable.

BigGillyStyle · 2017-05-25T09:44:53Z

Just as an FYI...we've had to revert to fog-core 1.44.1 gem version. 1.44.2 breaks with the following error:

/usr/local/rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/bundler-1.12.3/lib/bundler/runtime.rb:89:in rescue in block (2 levels) in require': There was an error while trying to load the gem 'fog'. (Bundler::GemRequireError) Gem Load Error is: couldn't find HOME environment -- expanding~'
Backtrace for gem load error is:
[release_directory]/vendor/bundle/ruby/2.3.0/gems/fog-core-1.44.2/lib/fog/core/cache.rb:94:in `expand_path'

Credit to @PashaLVWD for finding this for us. He led me to this PR, which may be a fix?

geemus · 2017-05-25T14:02:53Z

@BigGillyStyle Thanks for the heads up. I think this contains the fix (but also some other stuff which is still in progress). I'll see if I can't extract the relevant bit so we can cut that into a release.

geemus · 2017-05-25T14:07:43Z

@BigGillyStyle I think it should be fixed and released in v1.44.3, sorry for the breakage!

BigGillyStyle · 2017-05-25T18:41:36Z

@geemus No worries...I mostly wanted to just let you know about this. I just tested out 1.44.3 and we're all good. Awesome turnaround! 👍

shaiguitar · 2017-06-09T22:32:31Z

Been running this fork for a while and seems to work w/out an issue.

Added Fog::Cache.metadata for a given namespace so you can create custom cache expiry policies, or whatever you'd like to do for a given cache namespace / cloud vendor account-region etc.

For example:

  def renew_cache?
    last_refreshed = Fog::Cache.metadata[:last_refreshed]

    if last_refreshed.nil?
      # no metadata
      return true
    end

    if (Time.now - last_refreshed) > EXPIRE_TIME
      return true
    else
      return false
    end
  end
end

Added Fog::Cache.clean! as a helper method to blow away all local cache not just a specific namespace.
Removed the $HOME env expand path fix that has since been merged.
Refactored some code out of the large method into a separate check.

FWIW, locally the tests pass:

$ ruby -v
ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-darwin16]

$ bundle exec rake travis

# Running:

................................................................../users/shai/.fog-cache/for-service-user-region-foo/fog_subfogtestservice_mock/fog_subfogtestmodel
/users/shai/.fog-cache/for-service-user-region-foo
..[fog][WARNING] Found duplicate items in the cache. Expire all & refresh cache soon.
..................................................................................................S..........................................................

Finished in 7.095842s, 31.7087 runs/s, 38.6142 assertions/s.

225 runs, 274 assertions, 0 failures, 0 errors, 1 skips

You have skipped tests. Run with --verbose for details.

geemus · 2017-06-14T16:04:04Z

@shaiguitar there were some weird breaks on master, which I think are fixed now. Could you rebase? Hopefully that will get us back to a passing test suite here. Thanks!

coveralls · 2017-06-14T17:30:23Z

Coverage increased (+0.4%) to 74.207% when pulling e008cd1 on shaiguitar:cache_fixes into 6f3844e on fog:master.

shaiguitar · 2017-06-14T17:39:36Z

@geemus rebased. looks like travis is failing on https://stackoverflow.com/questions/3163641/get-a-class-by-name-in-ruby for 1.9.3 and 1.8.7 ... Are those deprecated or should I try to patch those?

coveralls · 2017-06-14T17:44:10Z

Coverage increased (+0.4%) to 74.207% when pulling 6b71530 on shaiguitar:cache_fixes into 6f3844e on fog:master.

coveralls · 2017-06-14T17:51:38Z

Coverage increased (+0.5%) to 74.263% when pulling a5613cc on shaiguitar:cache_fixes into 6f3844e on fog:master.

shaiguitar · 2017-06-14T18:03:50Z

So fixed the const_get compat and running into a different 1.8.7 compat issue with Array#sample. Is 1.8.7 supported or is that planned to be removed from travis?

geemus · 2017-06-14T18:06:51Z

I think if you rebase off master it should help. I fixed the const_get issue there, as well as other dependency issues (and got master passing for everything). I think those fixes are pretty good/clean (and are apt to conflict with these changes). Does that help/make sense?

shaiguitar · 2017-06-14T18:11:54Z

I thought I had rebased earlier but now I'm wondering if I screwed up the rebase - can you take a quick look at the branch's current state?

geemus · 2017-06-14T18:13:59Z

I see my changes in the - portion of the diffs, but it looks like post-rebase they have all been removed (ie things like the const_split_and_get method, which I added to deal with the const issue). So maybe the merge choices were off?

shaiguitar · 2017-06-14T18:21:32Z

Yeah at this point git diff origin/master points to that const get compat method that you added being removed, which ironically I added a similar one after rebasing and not including that in. Right now the only thing broken on travis is a Array#sample method that should be Array#choice 1.8.7. I could add another compat method for that if 1.8.7 is still to be around on CI. Once that specific branch issue is cleared I imagine it should go green assuming there's nothing else wrong with the rebase.

coveralls · 2017-06-14T18:40:58Z

Coverage increased (+0.4%) to 74.159% when pulling 7189f85 on shaiguitar:cache_fixes into 6f3844e on fog:master.

coveralls · 2017-06-15T16:01:07Z

Coverage increased (+0.5%) to 74.306% when pulling 0cb7a0e on shaiguitar:cache_fixes into 6f3844e on fog:master.

shaiguitar · 2017-06-15T16:04:42Z

@geemus Latest compat error was 1.8.7 does not take uniq with a block. After this latest method addition travis passes. Unless you think moving the various compatibility methods from the module into core_ext or some sort makes sense, this should be ready to merge.

geemus

Having all those changes dropped is kind of frustrating, makes me feel like I wasted some effort. Anyway, some small other feedback here. Thanks!

geemus · 2017-06-15T16:38:00Z

lib/fog/core/cache.rb

-        load_cache(path)[:attrs]
+      loaded = cache_files.map do |path|
+        if valid_for_load?(path)
+          model_klass = const_get_compat(load_cache(path)[:model_klass])


Does this need to be repeated? Shouldn't it be the same as the passed-in model_klass value that is already set in the method scope?

geemus · 2017-06-15T16:38:45Z

lib/fog/core/cache.rb

+        end
+      end.compact
+
+      path = pick_random(cache_files)


I wasn't clear on this, why does this use a random file?

geemus · 2017-06-15T16:40:26Z

spec/core/cache_spec.rb

@@ -35,9 +35,63 @@ def initialize(opts = {})
    example_cache = File.expand_path(Fog::Cache.namespace(Fog::SubFogTestModel, @service)).downcase
    expected_namespace = File.expand_path("~/.fog-cache/for-service-user-region-foo").downcase

+    puts example_cache


Could you remove these puts? I think I pulled them on master, as I presume they were just accidentally left behind?

geemus · 2017-06-15T16:45:29Z

spec/core/cache_spec.rb

@@ -101,7 +173,7 @@ def initialize(opts = {})

    id = SecureRandom.hex

-    # security groups on aws for eg can have the same identity group name 'default'.
+    # security gruops on aws for eg can have the same identity group name 'default'.


I think I fixed this typo in my master also, looks like that change also was dropped.

Metadata can be used to implement cahce expiry policies. For example, you have a cache expirey policy per-day. Fog::Cache.metadata = {:last_dumped => "Tuesday"} then next day, you re-run, check and expire the cache based on the data that exists for that specific cache store.

coveralls · 2017-06-15T19:19:22Z

Coverage increased (+0.6%) to 74.392% when pulling 882037a on shaiguitar:cache_fixes into 6f3844e on fog:master.

coveralls · 2017-06-15T19:20:29Z

Coverage increased (+0.6%) to 74.392% when pulling 9e8191e on shaiguitar:cache_fixes into 6f3844e on fog:master.

coveralls · 2017-06-15T19:25:28Z

Coverage increased (+0.6%) to 74.392% when pulling 9e8191e on shaiguitar:cache_fixes into 6f3844e on fog:master.

shaiguitar · 2017-06-15T19:30:30Z

@geemus Understood. Rebase went off due to many commits I hadn't squashed so I opt'ed to skip some and just fill in the differences manually, it was my mistake.

I fixed the recent comments - removed random sampling by simply finding the first cache entry that is valid and the puts/comment typo. I also moved model/collection detection out of the loop. Let me know if there's anything left outstanding.

coveralls · 2017-06-15T19:54:53Z

Coverage increased (+0.6%) to 74.445% when pulling 665b3d9 on shaiguitar:cache_fixes into 6f3844e on fog:master.

geemus · 2017-06-16T13:51:08Z

Thanks for working through that with me, looks good.

plribeiro3000 reviewed May 9, 2017

View reviewed changes

shaiguitar force-pushed the cache_fixes branch from 4407551 to 5abee4d Compare May 19, 2017 13:17

shaiguitar mentioned this pull request May 19, 2017

use homedirectory, not actual ~ as for cache sandbox. #208

Merged

shaiguitar force-pushed the cache_fixes branch from c416140 to e008cd1 Compare June 14, 2017 17:27

geemus requested changes Jun 15, 2017

View reviewed changes

shaiguitar force-pushed the cache_fixes branch from 0cb7a0e to 882037a Compare June 15, 2017 19:10

typo

9e8191e

shaiguitar force-pushed the cache_fixes branch from 1f7443d to 9e8191e Compare June 15, 2017 19:17

silence warnings on travis run.

665b3d9

geemus approved these changes Jun 16, 2017

View reviewed changes

geemus merged commit 71513c5 into fog:master Jun 16, 2017

shaiguitar deleted the cache_fixes branch June 18, 2017 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache fixes #209

Cache fixes #209

shaiguitar commented May 9, 2017

plribeiro3000 May 9, 2017

geemus May 18, 2017

BigGillyStyle commented May 25, 2017 •

edited

geemus commented May 25, 2017

geemus commented May 25, 2017

BigGillyStyle commented May 25, 2017

shaiguitar commented Jun 9, 2017

geemus commented Jun 14, 2017

coveralls commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

geemus commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

geemus commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 15, 2017

shaiguitar commented Jun 15, 2017 •

edited

geemus left a comment

geemus Jun 15, 2017

geemus Jun 15, 2017

geemus Jun 15, 2017

geemus Jun 15, 2017

coveralls commented Jun 15, 2017

coveralls commented Jun 15, 2017

coveralls commented Jun 15, 2017

shaiguitar commented Jun 15, 2017

coveralls commented Jun 15, 2017

geemus commented Jun 16, 2017

Cache fixes #209

Cache fixes #209

Conversation

shaiguitar commented May 9, 2017

plribeiro3000 May 9, 2017

Choose a reason for hiding this comment

geemus May 18, 2017

Choose a reason for hiding this comment

BigGillyStyle commented May 25, 2017 • edited

geemus commented May 25, 2017

geemus commented May 25, 2017

BigGillyStyle commented May 25, 2017

shaiguitar commented Jun 9, 2017

geemus commented Jun 14, 2017

coveralls commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

geemus commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

geemus commented Jun 14, 2017

shaiguitar commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 15, 2017

shaiguitar commented Jun 15, 2017 • edited

geemus left a comment

Choose a reason for hiding this comment

geemus Jun 15, 2017

Choose a reason for hiding this comment

geemus Jun 15, 2017

Choose a reason for hiding this comment

geemus Jun 15, 2017

Choose a reason for hiding this comment

geemus Jun 15, 2017

Choose a reason for hiding this comment

coveralls commented Jun 15, 2017

coveralls commented Jun 15, 2017

coveralls commented Jun 15, 2017

shaiguitar commented Jun 15, 2017

coveralls commented Jun 15, 2017

geemus commented Jun 16, 2017

BigGillyStyle commented May 25, 2017 •

edited

shaiguitar commented Jun 15, 2017 •

edited