Skip to content

Loading…

Changed `riak attach` to use a remsh #254

Merged
merged 6 commits into from

8 participants

@angrycub

This serves to insulate the running riak Erlang Virtual Machine from
accidental CTRL-C's causing it to halt. The original behavior has been
moved to riak attach-direct

@angrycub angrycub Changed `riak attach` to use a remsh...
This serves to insulate the running riak Erlang Virtual Machine from
accidental CTRL-C's causing it to halt.  The original behavior has been
moved to `riak attach-direct`
141bfa9
@evanmcc evanmcc commented on an outdated diff
rel/files/riak
@@ -23,6 +23,31 @@ RUNNER_LOG_DIR={{runner_log_dir}}
# Note the trailing slash on $PIPE_DIR/
PIPE_DIR={{pipe_dir}}
RUNNER_USER={{runner_user}}
+PLATFORM_DATA_DIR={{platform_data_dir}}#!/bin/sh
+# -*- tab-width:4;indent-tabs-mode:nil -*-
@evanmcc
evanmcc added a note

If we want to do this stuff, it belongs at the top, not at line 27.

That stuff is already at the top, looks like a bad merge/diff to me.

@angrycub
angrycub added a note

I think it's copypasta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanmcc evanmcc commented on an outdated diff
rel/files/riak
@@ -23,6 +23,31 @@ RUNNER_LOG_DIR={{runner_log_dir}}
# Note the trailing slash on $PIPE_DIR/
PIPE_DIR={{pipe_dir}}
RUNNER_USER={{runner_user}}
+PLATFORM_DATA_DIR={{platform_data_dir}}#!/bin/sh
+# -*- tab-width:4;indent-tabs-mode:nil -*-
+# ex: ts=4 sw=4 et
@evanmcc
evanmcc added a note

actually this whole area of the code might be duplication?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanmcc evanmcc commented on an outdated diff
rel/files/riak
((20 lines not shown))
+
+ attach)
+ # Make sure a node is running
+ RES=`ping_node`
+ ES=$?
+ if [ "$ES" -ne 0 ]; then
+ echo "Node is not running!"
+ exit $ES
+ fi
+
+ shift
+ NAME=`echo $NAME_ARG | sed 's/-s*name[ \t]+//g'`
+ exec $ERTS_PATH/erl -name c_$$_$NAME -hidden -remsh $NAME $COOKIE_ARG
+ ;;
+
+ console)
@evanmcc
evanmcc added a note

why is there so much new code here? it seems like console should not be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jaredmorrow

So if you look at the resulting file, there is two of everything. Take a look at your branch and re-push to this PR.

@jaredmorrow

Also, this might need to wait until after we branch 1.3 unless there is a bug associated that is a must-need.

@evanmcc

That looks a lot better.

I think that it can wait till after 1.3, although it's a pretty strong nice-to-have.

@jaredmorrow

So, if it is a nice-to-have, I'd be more inclined to +1 if the new functionality was something like attach-nice with attach staying the same for 1.3. Also, I'd hold off on any +1 for a test addition to the basic_command_line test in riak_test.

@evanmcc
@seancribbs

@evanmcc Disagree. In scenarios where disterl is unresponsive, remsh might refuse to connect but attach always has the pipes.

@angrycub

I'd just not like it to be the "go to" version of how we connect. I like the idea of keeping the piped version in, just as a different name. Working on some riak-tests now to test the behavior.

@evanmcc
@seancribbs

@evanmcc I know, I was more commenting on your suggestion to remove it entirely. Aside from the inability for Erlang to daemonize itself sensibly, having the run_erl/to_erl pair can be extremely helpful for support.

@evanmcc
@seancribbs

I might also mention that running q(). from the remsh still quits the Riak node. At least you won't hang it by hitting Ctrl-C, however.

@evanmcc
@jaredmorrow jaredmorrow referenced this pull request
Closed

Add daemontools support #265

@doubleyou

My 5 cents: #265 (comment)

@rzezeski

I vote we make attach use remsh by default, here are my reasons:

  1. If lager console is enabled and node is busy then it can be impossible to do anything because logging msgs clobber all your input/output.

  2. If you mess with any shared binaries or make other big structures the memory will continue to be used even after you detach.

  3. I recently watched a customer bring down multiple nodes by using Ctrl-C, and being fairly upset about it.

@doubleyou

I'll repeat myself here, but why use run_erl in the first place? Does it have any benefits except logs rotation?

@seancribbs

@doubleyou See my comment above. If disterl is unresponsive (busy_dist_port, e.g.), connecting to the fifos will still work.

@evanmcc

Any objections to merging this before the upcoming freeze? I note that it's still hanging out here, lacking consensus. I'd like to re-review the code, but my opinion is that it should go in.

@reiddraper

If we go with this, let's also update Riak Enterprise, Riak CS, Stanchion and any other projects which have an attach feature.

@jaredmorrow

@evanmcc I'd like to see a test added to https://github.com/basho/riak_test/blob/master/tests/basic_command_line.erl if new behavior and old behavior (moving run_erl to a different command) is tested and working, I'm +1. @reiddraper if it makes it in, I'll move it into node_package and add it to cs/stanchion/etc.

@jaredmorrow

I'll pick this up for testing.

@jaredmorrow jaredmorrow was assigned
@jaredmorrow

Wrote partial riak_test, but having trouble with some riak_test functionality. Tested a lot by hand, and added details to release notes. @angrycub if you can handle a PR to docs.basho.com to update any mention of riak attach I'll finish the riak_test. In the meantime, this can be merged.

@jaredmorrow jaredmorrow merged commit cb9e224 into basho:master
@jaredmorrow jaredmorrow added a commit to basho/riak_test that referenced this pull request
@jaredmorrow jaredmorrow Add functionality for remsh `riak attach` in Riak 1.4
Adds attach_direct function to correspond to `riak attach-direct`
as well as change the basic_command_line test to test both
`riak attach` and `riak attach-direct`.

Corresponds to functionality added in basho/riak#254
ead0217
@joedevivo

you broke my :heart: @angrycub, YOU BROKE MY :heart:!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 6, 2012
  1. @angrycub

    Changed `riak attach` to use a remsh...

    angrycub committed
    This serves to insulate the running riak Erlang Virtual Machine from
    accidental CTRL-C's causing it to halt.  The original behavior has been
    moved to `riak attach-direct`
  2. @angrycub

    Fixing copypasta error

    angrycub committed
  3. @angrycub
  4. @angrycub
  5. @angrycub

    Line ending cleanup.

    angrycub committed
  6. @angrycub
Showing with 17 additions and 2 deletions.
  1. +17 −2 rel/files/riak
View
19 rel/files/riak
@@ -228,7 +228,7 @@ case "$1" in
fi
;;
- attach)
+ attach-direct)
if [ "$2" = "-f" ]; then
echo "Forcing connection..."
else
@@ -245,6 +245,21 @@ case "$1" in
exec $ERTS_PATH/to_erl $PIPE_DIR
;;
+ attach)
+ # Make sure a node is running
+ RES=`ping_node`
+ ES=$?
+ if [ "$ES" -ne 0 ]; then
+ echo "Node is not running!"
+ exit $ES
+ fi
+
+ shift
+ NAME=`echo $NAME_ARG | sed 's/-s*name[ \t][ \t]*//g'`
+ exec $ERTS_PATH/erl -name c_$$_$NAME -hidden -remsh $NAME $COOKIE_ARG
+ ;;
+
+
console)
RES=`ping_node`
if [ "$RES" = "pong" ]; then
@@ -308,7 +323,7 @@ case "$1" in
echo $PID
;;
*)
- echo "Usage: $SCRIPT {start|stop|restart|reboot|ping|console|attach|chkconfig|escript|version|getpid}"
+ echo "Usage: $SCRIPT {start|stop|restart|reboot|ping|console|attach|attach-direct|chkconfig|escript|version|getpid}"
exit 1
;;
esac
Something went wrong with that request. Please try again.