Fix clean etc #104

dazza-codes · 2017-05-10T23:36:56Z

Might fix #83

solr_wrapper clean was often reporting that solr has to be stopped before cleaning it, when solr was never running in the first place. This bugged me often enough that it motived a fork and these changes. Throwing up this PR for consideration, maybe it's useful upstream.

Added some other command-line utils, including status and stop. They are working for me. No specs needed to change for this PR, everything was passing OK on my laptop.

Example of using this branch:

$ solr_wrapper
Starting Solr 6.5.1 on port 8983 ... http://127.0.0.1:8983/solr/
^Z
[1]+  Stopped                 solr_wrapper

$ bg
[1]+ solr_wrapper &

$ solr_wrapper status

Found 1 Solr nodes: 

Solr process 8460 running on port 8983
{
  "solr_home":"/tmp/solr-6.5.1/server/solr",
  "version":"6.5.1 cd1f23c63abe03ae650c75ec8ccb37762806cc75 - jimczi - 2017-04-21 12:23:42",
  "startTime":"2017-05-10T23:56:49.492Z",
  "uptime":"0 days, 0 hours, 0 minutes, 17 seconds",
  "memory":"18.3 MB (%3.7) of 490.7 MB"}

$ solr_wrapper stop
$ solr_wrapper status

No Solr nodes are running.

[1]+  Done                    solr_wrapper

cbeer · 2017-05-10T23:40:36Z

exe/solr_wrapper

@@ -1,5 +1,9 @@
 #!/usr/bin/env ruby

+if ENV['RAILS_ENV'] && ENV['RAILS_ENV'] == 'production'


This gem doesn't require rails, so a dependency on RAILS_ENV is inappropriate.

It doesn't depend on it, it just checks for an ENV value, that always works.

I don't think it's an appropriate check for this gem to make. While I think it's a bad idea for someone to want to do that, this gem has no place enforcing that.

cbeer · 2017-05-10T23:40:38Z

exe/solr_wrapper

-    $stderr.puts "Please stop solr before cleaning"
-    exit 1
-  end
+  return if ENV['RAILS_ENV'] && ENV['RAILS_ENV'] == 'production'


This gem doesn't use rails, so a dependency on RAILS_ENV is inappropriate.

cbeer · 2017-05-10T23:40:49Z

lib/solr_wrapper/instance.rb

+    ##
+    # Is Solr running?
+    def started?
+      status =~ /running on port #{port}/ && true || false


The boolean logic at the end makes no sense to me?

Personal preference, I like to see actual true or false instead of truthy or falsy values and this satisfies that craving.

Please change it for this project.

cbeer · 2017-05-10T23:41:57Z

lib/solr_wrapper/instance.rb


    ##
    # Check the status of a managed Solr service
    def status


This seems like a backwards-compatible change in behavior for #status? Could we extract this behavior to a different method name instead?

As it is only ever called locally and all those calls use started? instead, it doesn't matter (at least the specs don't care, so that's goog enough for me, esp. if this is all private anyway).

#status is in the public api for SolrWrapper::Instance, is it not?

Yea, so it is. But it could be private and seems to operate as private. I dunno. Maybe a major version change? I like the name status for this method. I started with status_info and then status did what started? does now and basically started? can be an alias for status. I could go back to that if this API is important to maintain.

cbeer · 2017-05-10T23:42:44Z

lib/solr_wrapper/instance.rb

+      exec('status').read
    rescue
-      false
+      'No status information available'


Can we provide a little more context in this error message? At the very least, knowing what we rescued could go a long way to aiding debugging.

It's not really an error message, it's a status message that there is not status info.

... which only happens in exceptional cases. I'm 👍 to giving users better access to this status information, but if we're going to do that, we should also give them enough information when things are going wrong.

cbeer · 2017-05-10T23:44:35Z

lib/solr_wrapper/instance.rb

    def clean!
      stop
      remove_instance_dir!
-      FileUtils.remove_entry(config.download_dir, true) if File.exist?(config.download_dir)


Instead of removing this line whole-sale, let's put it behind a named parameter flag (say, #clean! except_download_dir: true, or something better). There's a real use-case for "get rid of everything solr_wrapper has done", and I want to preserve that ability.

clean vs. clean! maybe? where the latter calls this clean and then also cleans up the downloads (all of them).

A ! suffix usually indicates dangerousness or mutating state, neither of which seem to be true here. I'd rather we make it an opt-in/-out flag.

cbeer · 2017-05-10T23:45:09Z

exe/solr_wrapper

+  return if ENV['RAILS_ENV'] && ENV['RAILS_ENV'] == 'production'
  $stderr.puts "cleaning #{instance.instance_dir}..."
-  instance.remove_instance_dir!
+  instance.clean!


I'm not clear on why this change was made?

The clean! calls stop so that's the big diff. Given that, then it's not necessary to check if it's stopped, it will get stopped.

cbeer · 2017-05-10T23:46:37Z

solr_wrapper.gemspec

  spec.add_development_dependency "bundler", "~> 1.7"
  spec.add_development_dependency "rake", "~> 10.0"

+  spec.add_development_dependency "pry"


Remove this commit. I'm not interested in this gem requiring pry for development.

- do not allow solr_wrapper in production, esp. clean - status returns an info string, use started? for boolean - status and started? do not check managed? because it's not reliable - clean command actually runs clean! - do not clean up the downloaded .zip file

…blic API

dazza-codes · 2017-05-11T00:48:12Z

Addressed comments, leaving commits for checking details, can squash as required later.

cbeer

It seems like this doesn't address some of the comments I made earlier. I'll review it again tomorrow.

…verything

jrochkind · 2017-05-22T14:19:30Z

This gem doesn't require rails, so a dependency on RAILS_ENV is inappropriate.

it is a very common practice for gems that don't depend on/require rails to still check for RAILS_ENV. Developers who are using Rails find it very convenient, and there's really no cost to the gem to do it.

Often gems will first check for a "${GEM_NAME}_ENV", lazily resorting to RAILS_ENV only if it's not present, so people not using Rails can still use an ENV setting without inaccurately calling it RAILS_ENV. Because having env-specific config is very common, even without Rails, and for those using Rails falling through to RAILS_ENV is very convenient.

I disagree with avoiding this develper convenience in the name of some kind of purity that has really no cost at all.

Examples: Many web servers use both RACK_ENV or RAILS_ENV, although they can work with any rack app, not just rails.

Sidekiq also uses RAILS_ENV, although it has no dependency on rails. https://github.com/mperham/sidekiq/blob/3b00587ea0b3f887df9053ee2f6b42a69481b5ce/lib/sidekiq/cli.rb#L211

An alternative might be allowing setting of 'ENV' in the .solr_wrapper file (which is run through erb), so the user could choose to set it to <%= ENV['RAILS_ENV'] %> -- but of course the problem is that under the common usage, which config file to load depends on the env, so it's too late.

jrochkind · 2017-05-22T14:22:47Z

That said, I'm not sure what this PR is doing with RAILS_ENV is what I'd do, but it still seems like progress to me. Things get very confusing at present, anything to reduce the confusion would be great.

In my ideal world, I'd have one config file that had env-specific development: test: etc keys, and have that convention established by this gem. Currently, without the gem supplying any support for this, different downstream consumers who still need env-specific config are left to their own devices, and what activefedora has decided, to put the development config in .solr_wrapper but the test config in config/test_solr_wrapper.yml is awfully confusing. While the downstream consumers could make differnet less confusing decisions -- they couldn't easily actually put everything in one file without support from the upstream solr_wrapper gem.

cbeer · 2017-05-22T17:11:24Z

lib/solr_wrapper/instance.rb

-      return true unless config.managed?
+    # Is Solr running?
+    def started?
+      status_info =~ /running on port #{port}/ && true || false


I commented on this previously. Please remove && true || false. It should be sufficient that started? return a truthy value. If that isn't the case, let's be explicit about that using match() and checking for a return value.

OK, deferring to your preference on this. Removing it. (Aside, I'm curious and open to using an explicit match() way of doing as an alternative coding style; rubocop doesn't approve of !!)

Actually, I'm going to push up another commit to replace this with an String#include? call instead, which seems cleaner and could be faster although that's not important here.

http://www.mervine.net/ruby-methods-vs-regex

cbeer · 2017-05-22T17:14:53Z

lib/solr_wrapper/instance.rb

-    # Clean up any files solr_wrapper may have downloaded
-    def clean!
+    # Clean up any files solr_wrapper may have downloaded, except the solr-{ver}.zip file
+    def clean


Here is our previous discussion:

#104 (comment)

@cbeer cbeer 12 days ago Owner Instead of removing this line whole-sale, let's put it behind a named parameter flag (say, #clean! except_download_dir: true, or something better). There's a real use-case for "get rid of everything solr_wrapper has done", and I want to preserve that ability. @darrenleeweber darrenleeweber 12 days ago clean vs. clean! maybe? where the latter calls this clean and then also cleans up the downloads (all of them). @cbeer cbeer 12 days ago Owner A ! suffix usually indicates dangerousness or mutating state, neither of which seem to be true here. I'd rather we make it an opt-in/-out flag.

I see you chose to use clean and clean! instead, which surprises me.

OK, pushing up something with #clean! clean_downloads: false, so the default action is not to clean up the downloads. The commit also focuses on removing files only (.md5, .zip download files) and not the entire download directory (in case anyone sets .solr_wrapper with a download_dir like ~/Downloads). The more likely scenario is that multiple projects use different versions of solr and share a download_dir, so any instance of solr_wrapper should clean up only the files configured for this instance (or project).

dazza-codes · 2017-05-22T18:34:23Z

$ bundle exec solr_wrapper -h
Usage: solr_wrapper [options] subcommand
    -v, --[no-]verbose               Run verbosely
        --config FILE                Load configuration from a file
        --version VERSION            Specify a Solr version to download (default: latest)
    -p, --port PORT                  Specify the port Solr should run at (default: 8983)
    -c, --cloud                      Run solr in cloud mode
        --solr_zip_path PATH         Download/use solr at the given path
    -i, --instance_directory DIR     Install/use solr at the given directory
    -l, --lib_directory DIR          Grab extra libs from this directory
    -n, --collection_name NAME       Create a default solr collection with the given name
    -d, --collection_config DIR      Create a default solr collection with the files from the given directory
        --[no-]persist               Persist the solr connection data
        --no-checksum                Skip running checksum validation on the downloaded file

Commonly used command are:
   clean  :    stop and clean data from solr, then create a new instance
   purge  :    as in 'clean', but this also removes solr download files
   dir    :    prints the solr instance dir
   status :    information about running solr cores
   stop   :    stop a running solr core
See 'solr_wrapper COMMAND --help' for more information on a specific command.

dazza-codes · 2017-05-22T19:18:11Z

Obviously some squashing is required before a merge. I'm only leaving individual commits for the sake of review in the PR, to help track the review requirements/discussion.

cbeer · 2017-05-22T21:44:39Z

exe/solr_wrapper

+  instance.clean!
+when 'purge'
+  $stderr.puts "cleaning solr downloads and solr instance: #{instance.instance_dir} ..."
+  instance.clean!(true)


Shouldn't this be instance.clean!(clean_downloads: true)?

dazza-codes · 2017-05-22T22:23:43Z

My ~/.solr_wrapper.yml uses download dir: ~/tmp/solr_wrapper

$ bundle exec solr_wrapper clean
cleaning solr instance: /tmp/solr-6.5.1 ...
$ ll ~/tmp/solr_wrapper/solr-6.5.1*
-rw-r--r-- 1 dlweber users 144M May  5 12:58 /users/dlweber/tmp/solr_wrapper/solr-6.5.1.zip
-rw-r--r-- 1 dlweber users   49 May 10 16:04 /users/dlweber/tmp/solr_wrapper/solr-6.5.1.zip.md5

$ bundle exec solr_wrapper purge
cleaning solr downloads and solr instance: /tmp/solr-6.5.1 ...
$ ll ~/tmp/solr_wrapper/solr-6.5.1*
ls: cannot access '/users/dlweber/tmp/solr_wrapper/solr-6.5.1*': No such file or directory

…oad file removal

dazza-codes · 2017-06-12T19:23:50Z

@cbeer - bump

dazza-codes · 2017-06-29T22:07:40Z

@cbeer - reminder....

cbeer reviewed May 10, 2017

View reviewed changes

dazza-codes force-pushed the fix-clean-etc branch from 98e783c to 5be1436 Compare May 11, 2017 00:18

dazza-codes added 4 commits May 10, 2017 17:37

SolrWrapper::Instance - clean! calls clean to clean up everything

71534b3

exe/solr_wrapper - remove checks on RAILS_ENV

92b524d

SolrWrapper::Instance - status reports exception details

eef5040

SolrWrapper::Instance - add status_info, preserve status and other pu…

d684ae9

…blic API

dazza-codes force-pushed the fix-clean-etc branch from 47b41ac to d684ae9 Compare May 11, 2017 00:46

cbeer requested changes May 11, 2017

View reviewed changes

exe/solr_wrapper - update use information and add "clean!" to purge e…

0ac9704

…verything

cbeer reviewed May 22, 2017

View reviewed changes

dazza-codes added 3 commits May 22, 2017 15:26

SolrWrapper::Instance#clean! - clean_downloads param can toggle downl…

8d83799

…oad file removal

SolrWrapper::Instance#started? returns truthy regex match

417c744

SolrWrapper::Instance#started? returns boolean using String#include?

d5106cf

dazza-codes force-pushed the fix-clean-etc branch from f09af9b to d5106cf Compare May 22, 2017 22:26

		@@ -1,5 +1,9 @@
		#!/usr/bin/env ruby

		if ENV['RAILS_ENV'] && ENV['RAILS_ENV'] == 'production'

Fix clean etc #104

Are you sure you want to change the base?

Fix clean etc #104

Uh oh!

Conversation

dazza-codes commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dazza-codes commented May 11, 2017

Uh oh!

cbeer left a comment

Choose a reason for hiding this comment

Uh oh!

jrochkind commented May 22, 2017

Uh oh!

jrochkind commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dazza-codes commented May 22, 2017

Uh oh!

dazza-codes commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dazza-codes commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dazza-codes commented Jun 12, 2017

Uh oh!

dazza-codes commented May 10, 2017 •

edited

Loading

dazza-codes commented May 22, 2017 •

edited

Loading