Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix passing of empty string arg to riak rpc in riak admin services #11

Merged

Conversation

hmmr
Copy link

@hmmr hmmr commented Nov 2, 2024

This should fix #8.

@martinsumner
Copy link

Changing the rpc() function for the case where there are no args beyond the function would work as well ...

i.e.

rpc() {
    local mod=$1
    local fun=$2
    shift 2
    if [ $# -gt 0 ]; then
        "${PLATFORM_BIN_DIR}/riak" rpc $mod $fun `fmt "$@"`
    else
        "${PLATFORM_BIN_DIR}/riak" rpc $mod $fun "[[]]"
    fi
}
rpc() {
    local mod=$1
    local fun=$2
    shift 2
    if [ $# -gt 0 ]; then
        "${PLATFORM_BIN_DIR}/riak" rpc $mod $fun `fmt "$@"`
    else
        "${PLATFORM_BIN_DIR}/riak" rpc $mod $fun "$3"
    fi
}

But do we fear that this will simply cause something else to fail?

@hmmr
Copy link
Author

hmmr commented Nov 4, 2024

Indeed, replacing "[[]]" with "$3" will break commands such as riak admin reformat-indexes, for which an explicit [] is required as the underlying riak_kv_console:reformat_indexes is /1. Without that, rebar machinery will try to invoke riak rpc with two arguments, and result in a call to riak_kv_console:reformat_indexes/0:

$ ./rel/riak/bin/riak admin reformat-indexes
RPC to '[email protected]' failed: {'EXIT',
                                 {undef,
                                  [{riak_kv_console,reformat_indexes,[],[]}]}}

On the other hand, riak_core_node_watcher:servives is arity 0, and for this case, "" (and not "[[]]") is appropriate.

There was a change in argument passing conventions in rebar-3.18. Exactly how it all worked before, is a good question.

So yes, replacing rpc with rpc_raw for riak admin services is the minimal sufficient change.

@martinsumner
Copy link

Thanks for the explanation.

Going forward we need to get this into openriak-3.2 and openriak-3.4 branches. There's a team call on Wednesday, we can use this change as a test case as to how we agree changes on those branches.

Heads up @tburghart

@martinsumner martinsumner merged commit c71317d into nhse-develop Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants