Enable caching on connections #214

jquick · 2017-11-17T17:06:19Z

This changes adds a new connection called "CacheConnection". When passed a connection it will cache file and command calls. All connection methods will be accessible on the cached version.

Signed-off-by: Jared Quick jquick@chef.io

adamleff

@jquick I really like the shape of this. I know this isn't exactly what @chris-rock was pitching initially, but after you and I talked about it, I think this is a better approach. It ensures that we don't have to do any Ruby magic to make the "Cache" connection have all the instance messages for the connection it represents, and it cleanly extends caching to ANY connection that subclasses BaseConnection. I think this is a more Rubyish way to accomplish this. Nice work.

I have some cleanup items and suggestions for you to review, and we can talk to @chris-rock more about this next week.

adamleff · 2017-11-17T21:22:50Z

lib/train/plugins/base_connection.rb

+    # @param command [String] command string to execute
+    # @return [CommandResult] contains the result of running the command
+    def run_command_via_connection(_command)
+      fail Train::ClientError, "#{self.class} does not implement #run_command_via_connection()"


I think we should raise a NotImplementedError instead as that's the standard exception class for this type of issue. Is there a reason we shouldn't?

adamleff · 2017-11-17T21:23:02Z

lib/train/plugins/base_connection.rb

-    def file(_path, *_args)
-      fail Train::ClientError, "#{self.class} does not implement #file(...)"
+    def file_via_connection(_path, *_args)
+      fail Train::ClientError, "#{self.class} does not implement #file_via_connection(...)"


I think we should raise a NotImplementedError instead as that's the standard exception class for this type of issue. Is there a reason we shouldn't?

adamleff · 2017-11-17T21:24:08Z

lib/train/plugins/cache_connection.rb

+      }
+
+      @cache_enabled.each_key do |type|
+        @cache[type] = {}


Maybe call clear_cache here instead?

adamleff · 2017-11-17T21:25:19Z

lib/train/plugins/cache_connection.rb

+
+class Train::Plugins::Transport
+  class CacheConnection
+    attr_accessor :cache_enabled


Ruby nitpick, I'd rather see a cache_enabled? method that looks at this hash and returns true/false... just to keep consistency with ? methods returning booleans. If you choose to take this advice, this will obviously require some changes in BaseConnection, but not too many.

adamleff · 2017-11-17T21:27:06Z

lib/train/plugins/cache_connection.rb

+    end
+
+    def file(path)
+      if @cache[:file].key?(path)


Could this be simplified to?:

@cache[:file][path] ||= @connection.file_via_connection(path)

That should return the value if it's there, otherwise, assign the new value and then return it.

adamleff · 2017-11-17T21:27:20Z

lib/train/plugins/cache_connection.rb

+    end
+
+    def run_command(cmd)
+      if @cache[:command].key?(cmd)


Same as above - I think we can do ||= and simplify a bit.

adamleff · 2017-11-17T21:29:53Z

test/unit/plugins/connection_test.rb

@@ -14,6 +14,10 @@
      proc { connection.run_command('') }.must_raise Train::ClientError
    end

+    it 'provides a run_command_via_connection method' do


I wonder if this should be it 'raises an exception for run_command_via_connection', and then we add a it 'provides a run_command_via_connection method' check for each of our connection classes. Thoughts?

adamleff · 2017-11-17T21:29:58Z

test/unit/plugins/connection_test.rb

@@ -22,6 +26,10 @@
      proc { connection.file('') }.must_raise Train::ClientError
    end

+    it 'provides a file_via_connection method' do
+      proc { connection.file_via_connection('') }.must_raise Train::ClientError


Same as above...

jquick · 2017-11-22T02:18:17Z

This should be updated with all changes to date.

chris-rock

I think we are very close @jquick

chris-rock · 2017-11-22T15:53:11Z

lib/train/plugins/base_connection.rb

+    #
+    # @param command [String] command string to execute
+    # @return [CommandResult] contains the result of running the command
+    def run_command_via_connection(_command)


Those two methods need to be private

Yes, moved.

chris-rock · 2017-11-22T15:54:23Z

lib/train/transports/docker.rb

-        else
-          Train::File::Remote::Linux.new(self, path)
-        end
+    def uri


Is there a reason to make this public?

I think we need it for now as inspec calls it here: https://github.com/chef/inspec/blob/master/lib/inspec/rspec_json_formatter.rb#L561

Missed it, thank you.

chris-rock · 2017-11-22T15:55:11Z

lib/train/transports/local.rb

+        nil # none, open your shell
+      end
+
+      def uri


Why are we exposing this one?

chris-rock · 2017-11-22T15:56:32Z

lib/train/transports/mock.rb

+    end
+
+    def file_via_connection(path)
+      @files[path] ||= file_not_found(path)


Should we use the file cache and just fill the cache? So this will become a noop then

Signed-off-by: Jared Quick <jquick@chef.io>

jquick · 2017-11-27T03:09:09Z

This is updated with all changes.

chris-rock

This looks great, I added some further comments. Would love to see this in action with https://github.com/chef/inspec/pull/2309/files before we merge. @jquick are you updating inspec/inspec#2309

chris-rock · 2017-11-27T09:42:01Z

lib/train/plugins/base_connection.rb

+      @cache_enabled[type.to_sym]
+    end
+
+    def enable_cache(type)


I think we need a comment to document which types are available.

chris-rock · 2017-11-27T09:43:27Z

lib/train/plugins/transport.rb

@@ -1,7 +1,4 @@
 # encoding: utf-8
-#
-# Author:: Dominik Richter (<dominik.richter@gmail.com>)


I am not sure why this is removed?

It was a directive via Adam to remove any author tags on files we touch. I can add it back for now.

Signed-off-by: Jared Quick <jquick@chef.io>

jquick · 2017-11-27T12:13:41Z

This has been updated with changes. inspec/inspec#2309 was also updated to use this new functionality.

chris-rock

Great work @jquick

chris-rock · 2017-11-27T15:02:49Z

lib/train/transports/docker.rb

-        else
-          Train::File::Remote::Linux.new(self, path)
-        end
+    def uri


Missed it, thank you.

adamleff

This is so freakin' close. I have a suggestion on making the run_command and file method just read a bit cleaner and remove some logic when returning.

adamleff · 2017-11-27T15:28:55Z

lib/train/plugins/base_connection.rb

+    # Enable caching types for Train. Currently we support
+    # :file and :command types
+    def enable_cache(type)
+      @cache_enabled[type.to_sym] = true


Future enhancement: raise an exception if the user tries to enable/disable a cache that we don't support.

+1 great catch!

adamleff · 2017-11-27T15:31:19Z

lib/train/plugins/base_connection.rb

+    # This is the main command call for all connections. This will call the private
+    # run_command_via_connection on the connection with optional caching
+    def run_command(cmd)
+      return @cache[:command][cmd] ||= run_command_via_connection(cmd) if cache_enabled?(:command)


I think it might be a bit cleaner to read if it was structured to do the non-cache enabled stuff first:

return run_command_via_connection(cmd) unless cache_enabled?(:command) @cache[:command][cmd] ||= run_command_via_connection(cmd)

The return combined with the ||= is hanging me up a bit. Feels like too much happening in one conditional statement.

Uh, I like that one. Thank you @adamleff

adamleff · 2017-11-27T15:31:51Z

lib/train/plugins/base_connection.rb

+    # This is the main file call for all connections. This will call the private
+    # file_via_connection on the connection with optional caching
+    def file(path, *args)
+      return @cache[:file][path] ||= file_via_connection(path) if cache_enabled?(:file)


Same feedback as the run_command structure feedback.

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff · 2017-11-27T17:01:41Z

lib/train/plugins/base_connection.rb

@@ -43,10 +43,12 @@ def cache_enabled?(type)
    # Enable caching types for Train. Currently we support
    # :file and :command types
    def enable_cache(type)
+      fail Train::UnknownCacheType, "#{type} is not a valid cache type" unless @cache_enabled.keys.include?(type.to_sym)


Rather than getting a list of the keys, you can ask the Hash if it has the key:

... unless @cache_enabled.key?(type.to_sym)

adamleff

Nice work, @jquick :)

jquick requested review from chris-rock, a team and adamleff November 17, 2017 17:06

jquick force-pushed the jq/cache_connection branch 3 times, most recently from 9fa599f to b4a4785 Compare November 17, 2017 20:33

adamleff suggested changes Nov 17, 2017

View reviewed changes

jquick force-pushed the jq/cache_connection branch 4 times, most recently from 53b774c to e853f12 Compare November 21, 2017 20:29

chris-rock reviewed Nov 22, 2017

View reviewed changes

jquick added 5 commits November 26, 2017 20:30

Add cache connection to train

e58f229

Signed-off-by: Jared Quick <jquick@chef.io>

Update caching to be on BaseConnection

5f1ede5

Signed-off-by: Jared Quick <jquick@chef.io>

Refactor tests and cleanup logic

a1784d8

Signed-off-by: Jared Quick <jquick@chef.io>

Move caching into base_connection and privatize connection calls

31214ba

Signed-off-by: Jared Quick <jquick@chef.io>

Update mock transport to use base caching.

231bae3

Signed-off-by: Jared Quick <jquick@chef.io>

jquick force-pushed the jq/cache_connection branch from e853f12 to 231bae3 Compare November 27, 2017 03:03

chris-rock reviewed Nov 27, 2017

View reviewed changes

Add author tag and caching comments.

1dba81b

Signed-off-by: Jared Quick <jquick@chef.io>

chris-rock approved these changes Nov 27, 2017

View reviewed changes

lib/train/transports/docker.rb

else

Train::File::Remote::Linux.new(self, path)

end

def uri

Copy link

Contributor

chris-rock Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed it, thank you.

adamleff suggested changes Nov 27, 2017

View reviewed changes

Added exception for invalid cache type and small logic refactor.

b8cd014

Signed-off-by: Jared Quick <jquick@chef.io>

adamleff reviewed Nov 27, 2017

View reviewed changes

adamleff approved these changes Nov 27, 2017

View reviewed changes

adamleff merged commit 00a3411 into master Nov 27, 2017

adamleff deleted the jq/cache_connection branch November 27, 2017 17:03

adamleff mentioned this pull request Dec 5, 2017

[win-perf] reduce number of powershell commands inspec/inspec#2158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable caching on connections #214

Enable caching on connections #214

jquick commented Nov 17, 2017

adamleff left a comment

adamleff Nov 17, 2017

adamleff Nov 17, 2017

adamleff Nov 17, 2017

adamleff Nov 17, 2017

adamleff Nov 17, 2017

adamleff Nov 17, 2017

adamleff Nov 17, 2017

adamleff Nov 17, 2017

jquick commented Nov 22, 2017 •

edited

Loading

chris-rock left a comment

chris-rock Nov 22, 2017

jquick Nov 27, 2017

chris-rock Nov 22, 2017

jquick Nov 27, 2017

chris-rock Nov 27, 2017

chris-rock Nov 22, 2017

chris-rock Nov 22, 2017

jquick commented Nov 27, 2017

chris-rock left a comment

chris-rock Nov 27, 2017

chris-rock Nov 27, 2017

jquick Nov 27, 2017

jquick commented Nov 27, 2017

chris-rock left a comment

chris-rock Nov 27, 2017

adamleff left a comment

adamleff Nov 27, 2017

chris-rock Nov 27, 2017

adamleff Nov 27, 2017

chris-rock Nov 27, 2017

adamleff Nov 27, 2017

adamleff Nov 27, 2017

adamleff left a comment

Enable caching on connections #214

Enable caching on connections #214

Conversation

jquick commented Nov 17, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick commented Nov 22, 2017 • edited Loading

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick commented Nov 27, 2017

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick commented Nov 27, 2017

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

jquick commented Nov 22, 2017 •

edited

Loading