Pull request

David Smith dizzyd at basho.com
Tue Dec 29 10:46:05 EST 2009


Kirill,

Thanks for the feedback and patches. In the future, it would be helpful if you
could provide rationale for large changesets such as the refactoring of the
scripts. It can difficult to understand the "why" when so much code is
affected; we can do a better job of integrating patches when we understand what
the author intended to accomplish.

All of that said, I've attempted to do a detailed breakdown of your changesets
and provide some (hopefully) helpful feedback.

* Moving common code to riak-common
  We can't depend on $(dirname $0) to do the right thing if repackaged for
  DEB/RPMS/etc. Perhaps some of the common logic could be moved IIF we maintain
  the base variable defs so that we know where to find riak-common.
This is worth
  considering and I will look into it.

* Using backticks vs $(..) form
  I noticed you replaced a number of backticks with this other form --
is there an
  advantage to this?

* Removing wait for process to fully exit
  Blocking on the server process to exit is highly desirable in production
  environments. Why change it? Also, what value do the async versions of
  stop/restart/reboot add? Finally, timeouts on the RPC calls are best practice
  in that they ensure you get a useful error if the server times out handling
  your RPC request.

* Nodetool is meant to be generic
  Nodetool provides a standard set of services for any Erlang embedded node and
  is not meant to be customized such as you did for
backup/restore/logger. I concur
  that the way we invoke those facilities is a little clunky at the
moment. However,
  customizing nodetool is not the right approach.

* Moving user to run as to vm.args
  Again, when packaging for DEB/RPMS/etc, the user to run as is likely not
  something one would want to be configurable by the end user -- this is why I
  placed it in the riak script as a variable. The intention is that the riak
  script, like nodetool, is meant to be generic. When being repackaged for a
  distributions, the repackaging tool can do a simple SED on the variables at
  the top of the riak script and get a fully customized startup script
  appropriate for a given distribution.

* default.config|args
  I'm not too keen on changing the names of these files from
app.config and vm.args to
  NODE.[config|args] for a few reasons:
  1. Changing the name of the config files does not also ensure that logging and
     data directories are also appropriately seperated. It would be easy for
     people to think they could just rename/copy the a set of config files and
     get seperate nodes -- this would not necssarily be true.
  2. The use case for running multiple instances of Riak on a single machine is
     not a common one. We provide in the top-level Riak Makefile a target for
     constructing multiple nodes (devrel) which is useful for
developers. However,
     in production, there will typically only be one software node per machine.
  3. Multiple copies of config files increase the possibility of production
     running a different configuration than what the operator _thinks_ they are
     running.

* Addition of use_fallback_nodes
  It's not entirely clear to me why one would want to disable the use of
  fallback nodes. This would have the effect of removing a key layer
of replication
  when the network splits or nodes otherwise become unavailable. Out
of curiosity,
  what's your use case here?

Thanks,

D.




More information about the riak-users mailing list