Replaced most if not all instances of 'nodes' with 'number_of_nodes'

[scouttyg]

I believe two major issues exist in the current gem:

1. The nodes argument is being improperly used when loading clusters, rather than number_of_nodes
2. running? needs to match parameters from what was initialized to return a truthy value.

For 1, let's take a quick look at the initialize method:

def initialize(arguments={})
  @arguments = arguments.dup

  @arguments[:command]           ||= ENV.fetch('TEST_CLUSTER_COMMAND',   'elasticsearch')
  @arguments[:port]              ||= ENV.fetch('TEST_CLUSTER_PORT',      9250).to_i
  @arguments[:cluster_name]      ||= ENV.fetch('TEST_CLUSTER_NAME',      __default_cluster_name).chomp
  @arguments[:node_name]         ||= ENV.fetch('TEST_CLUSTER_NODE_NAME', 'node')
  @arguments[:path_data]         ||= ENV.fetch('TEST_CLUSTER_DATA',      '/tmp/elasticsearch_test')
  @arguments[:path_work]         ||= ENV.fetch('TEST_CLUSTER_TMP',       '/tmp')
  @arguments[:path_logs]         ||= ENV.fetch('TEST_CLUSTER_LOGS',      '/tmp/log/elasticsearch')
  @arguments[:es_params]         ||= ENV.fetch('TEST_CLUSTER_PARAMS',    '')
  @arguments[:multicast_enabled] ||= ENV.fetch('TEST_CLUSTER_MULTICAST', 'true')
  @arguments[:timeout]           ||= ENV.fetch('TEST_CLUSTER_TIMEOUT',   60).to_i
  @arguments[:timeout_version]   ||= ENV.fetch('TEST_CLUSTER_TIMEOUT_VERSION', 15).to_i
  @arguments[:number_of_nodes]   ||= ENV.fetch('TEST_CLUSTER_NODES',     2).to_i
  @arguments[:network_host]      ||= ENV.fetch('TEST_CLUSTER_NETWORK_HOST', __default_network_host)
  @arguments[:quiet]             ||= ! ENV.fetch('QUIET', '').empty?

  @clear_cluster = !!@arguments[:clear_cluster] || (ENV.fetch('TEST_CLUSTER_CLEAR', 'true') != 'false')

  # Make sure `cluster_name` is not dangerous
  raise ArgumentError, "The `cluster_name` argument cannot be empty string or a slash" \
    if @arguments[:cluster_name] =~ /^[\/\\]?$/
end

The initialize method is called when Cluster.new is called, which is in turn called in methods start, stop, running?, wait_for_green, among others. So whenever any of these methods get passed arguments, it in turn passes those same arguments to the initialize method. But looking above, we can see that initialize doesn't handle any sort of arguments[:nodes], which I think was a relic of the arguments of previous generations of the gem.

For 2, let's take a quick look at the two running? methods:

def running?(arguments={})
  Cluster.new(arguments).running?
end

So again, this will call the initializer, and then run the other running? method:

def running?
  if cluster_health = Timeout::timeout(0.25) { __get_cluster_health } rescue nil
    return cluster_health['cluster_name']    == arguments[:cluster_name] && \
           cluster_health['number_of_nodes'] == arguments[:number_of_nodes]
  end
  return false
end

Now lets take the following code

Elasticsearch::Extensions::Test::Cluster.start(nodes: 1) if ENV['SERVER'] and not Elasticsearch::Extensions::Test::Cluster.running?

For one, as explained above, this wouldn't start a cluster with 1 node -- since arguments[:nodes] is not a thing, it defaults to the user's TEST_CLUSTER_NODES ENV variable, or 2.

But this actually silently makes this code work -- when no parameters are given to the running? method, it will use the default arguments as well, which again is 2 nodes.

When we fix this however, by changing it to the right code:

Elasticsearch::Extensions::Test::Cluster.start(number_of_nodes: 1) if ENV['SERVER'] and not Elasticsearch::Extensions::Test::Cluster.running?

This makes the code fail, because when no parameters are passed to the running? method, when it calls the Cluster.new method it will again default without a number_of_nodes argument to 2, and then when it does the check in the other running? method, it will be:

return cluster_health['cluster_name']    == arguments[:cluster_name] && \
           cluster_health['number_of_nodes'] == arguments[:number_of_nodes]

cluster_health['number_of_nodes'] will be 1, because we initialized it properly when we started it, but arguments[:number_of_nodes] will be 2, because we didn't pass in an argument there and so it set it to the default.

So in general, anytime we call the running? method, we need to make sure we pass it the same arguments we pass the start method, or it won't return the correct results I believe.

Hope that makes sense, let me know what people's thoughts are.

[scouttyg]

Any thoughts on this? Just wanted to check back in. Looks like it failed a check from Travis, but one that might be failing unrelated to this change. Let me know! 😃

[scouttyg]

Hey, me again, just checking back in a few months later to see if this still is valuable

[karmi]

Hi @scouttyg, my apologies for such a ridiculous delay on the patch... allow me to look into that, I do think there's some discrepancy there with the naming...

[karmi]

Hi @scouttyg, I've looked into the code, and indeed, these have been left over after the changes introduced in 4e6e6d2, and it was "silently working"...

Many thanks for catching it, I've merged it into the master and 5.x branches.