Add logic to clear cancelled connection requests #598

bsparrow435 · 2014-06-02T17:20:20Z

This branch adds logic to clear cancelled connections from the
riak_core_connection_manager Pending state. I have also added a few
helper functions to give a better view into request
states(get_request_states) and connection
errors(get_connection_errrors).

Connections are cancelled only when a user runs riak-repl disconnect NAME/[IP,PORT]. I’ve added logic to this path to call
remove_cancelled_connections after waiting cm_cancellation_interval.
This interval must either be an integer or the atom never.

This logic only makes one attempt to remove the cancelled connection.
It might be nice to have a helper function that we can call to manually
clear the connections but currently this can be done by re-trying
riak-repl disconnect.

The riak_test showing this issue is replication2_connections.erl and
the issue can be demonstrated with this code by setting the
cm_cancellation_interval to never.

This branch adds logic to clear cancelled connections from the riak_core_connection_manager Pending state. I have also added a few helper functions to give a better view into request states(get_request_states) and connection errors(get_connection_errrors). Connections are cancelled only when a user runs `riak-repl disconnect NAME/[IP,PORT]`. I’ve added logic to this path to call remove_cancelled_connections after waiting cm_cancellation_interval. This interval must either be an integer or the atom never. This logic only makes one attempt to remove the cancelled connection. It might be nice to have a helper function that we can call to manually clear the connections but currently this can be done by re-trying `riak-repl disconnect`. The riak_test showing this issue is `replication2_connections.erl` and the issue can be demonstrated with this code by setting the `cm_cancellation_interval` to `never`.

cmeiklejohn · 2014-06-02T17:30:36Z

Dialyzer is returning the following problems:

riak_core_connection_mgr.erl:541: The pattern 'ok' can never match the type {'error',atom()}

bsparrow435 · 2014-06-02T17:35:21Z

Oh god, the connection_helper, i hate this stupid function. It's supposed to start and then when finished hit the 'EXIT' handle_info in the riak_core_connection_mgr to iterate stats and do some other logic. However, i've seen it totally skip this when riak_core_connection:sync_connect returns an {error, Reason} which is one reason the stats module is useless. This helper function needs to be totally reworked.

With that said, the call at that line is riak_core_connection:sync_connect who's spec is -spec sync_connect(ip_addr(), clientspec()) -> ok | {error, atom()}.

I don't see the issue here.

cmeiklejohn · 2014-06-02T18:03:34Z

Test for this behavior: basho/riak_test#626

cmeiklejohn · 2014-06-04T19:34:43Z

@bsparrow435 How are we looking here?

cmeiklejohn · 2014-06-04T20:39:23Z

src/riak_core_connection_mgr.erl

            %% The helper process will discover the cancellation when it asks if it
            %% should connect to an endpoint.
            State#state{pending = lists:keystore(Req#req.ref, #req.ref, Pending,
                                                 Req#req{state = cancelled})}
    end.

+%% remove pending connection from state for supplied Ref.


Missing @doc.

Looking at the rest of the module, most functions are missing @doc and type specifications. I'll add these to my new functions but would it be ok to go ahead and add the rest in this PR?

Changes the atom for canceling cleared connections from `never` to `undefined`. Cleaned up logic and added `@doc` to one new function.

bsparrow435 · 2014-06-04T21:06:31Z

Just pushed commit addressing comments. Working on associated riak_test now.

cmeiklejohn · 2014-06-04T22:13:02Z

src/riak_core_connection_mgr.erl

            %% The helper process will discover the cancellation when it asks if it
            %% should connect to an endpoint.
            State#state{pending = lists:keystore(Req#req.ref, #req.ref, Pending,
                                                 Req#req{state = cancelled})}
    end.

+%% @doc remove pending connection from state for supplied Ref.
+remove_cancelled_connection(Ref, State = #state{pending = Pending}) ->
+    case lists:keytake(Ref, #req.ref, Pending) of


I still don't see how this will ever match, given that the second parameter is supposed to be an integer. Does this grab the default value from the record?

I did it this way because a few lines above we did the same thing in lists:keystore. My understanding is that #req.ref will return an integer representing it's place in the record. I'll try testing that now to confirm.

Looking at the rest of the module and other modules in riak_repl this is fairly common in all lists:key* operations.

I just confirmed this behavior with the following immediately below line 431 of this file:

lager:notice("BRIAN RETURN: ~p",[#req.ref]),

Result:

2014-06-04 18:50:57.294 [notice] <0.2467.0>@riak_core_connection_mgr:disconnect_from_target:431 BRIAN RETURN: 2

Ah, very interesting. Thanks for taking the time to explain why it works.

No big deal now, but that is definitely something I've never seen before. It's clever, but I think too much so.

I went to the docs, from the bottom of the Records page: http://www.erlang.org/doc/reference_manual/records.html

In addition, #Record.Name returns the index in the tuple representation of Name of the record Record. Name must be an atom.

So this is an explicit, by design behavior.

Seems the reason this works is because people were tired of defining indexes for elements in a record.

Yes, I know it's by design. I was arguing it was obtuse, given two Erlang programmers who have been doing it for 2+ years didn't know what it was doing, so we were making an argument to just put the integer value in (or macro).

Yep, and I know this is code you inherited, @bsparrow435, so not asking for it to be changed now. Might as well be consistent within the module until we fix the whole mess.

Just leave it as is and we'll address all of it later.

cmeiklejohn · 2014-06-04T23:05:40Z

I'm still concerned given this code introduced a new Dialyzer error which is not on master which should be fixed prior to merge. Otherwise, once the Dialyzer warning is fixed, and the test refactored, we're good to go here.

bsparrow435 · 2014-06-05T00:36:14Z

I just found an issue where, if connected/disconnected in the correct order within 5 minutes, the endpoint(ex. "127.0.0.1":"10046") will be blacklisted forever. I need to add some logic to reset the retry_backoff. Will work on it tonight.

bsparrow435 · 2014-06-05T00:36:27Z

Also will be adding another riak_test to show issue

bsparrow435 · 2014-06-05T01:05:38Z

False alarm, i wasn't waiting long enough for the endpoint backoff to finish. Still, going to add one more step to the riak_test to prove reconnection is possible as well as adding logic to reset the backoff timer when the cancelled connection is removed.

bsparrow435 · 2014-06-05T18:43:15Z

Dialyzer issue is the last open item:

riak_core_connection_mgr.erl:546: The pattern 'ok' can never match the type {'error',atom()}

Myself and @cmeiklejohn are looking into it now.

cmeiklejohn · 2014-06-05T18:56:34Z

@reiddraper found it; warning was is in the ignore file, but line number changed. Just push a commit to update the line number.

bsparrow435 · 2014-06-05T19:08:36Z

Nice find, thanks.

cmeiklejohn · 2014-06-05T19:22:29Z

Pushed d61ff7e with updated line numbers.

cmeiklejohn · 2014-06-05T19:22:48Z

👍 d61ff7e

…ions Add logic to clear cancelled connection requests Reviewed-by: cmeiklejohn

cmeiklejohn · 2014-06-05T19:40:50Z

@borshop merge

This was referenced Jun 2, 2014

Disconnect and cancelled connection tests basho/riak_test#626

Merged

Connection manager forever grows cancelled connections #586

Closed

cmeiklejohn added this to the 2.0-RC milestone Jun 2, 2014

cmeiklejohn added resilience labels Jun 2, 2014

cmeiklejohn self-assigned this Jun 2, 2014

cmeiklejohn reviewed Jun 4, 2014
View reviewed changes

Addressing PR comments

3612f9c

Changes the atom for canceling cleared connections from `never` to `undefined`. Cleaned up logic and added `@doc` to one new function.

cmeiklejohn reviewed Jun 4, 2014
View reviewed changes

Reset endpoint backoff during connect

bc6a51e

Update dialyzer.ignore-warnings based on new lines.

d61ff7e

borshop added a commit that referenced this pull request Jun 5, 2014

Merge pull request #598 from basho/bugfix/bjs/clear-cancelled-connect…

dd6ec07

…ions Add logic to clear cancelled connection requests Reviewed-by: cmeiklejohn

borshop merged commit d61ff7e into develop Jun 5, 2014

cmeiklejohn deleted the bugfix/bjs/clear-cancelled-connections branch June 5, 2014 20:05

bsparrow435 unassigned cmeiklejohn Aug 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logic to clear cancelled connection requests #598

Add logic to clear cancelled connection requests #598

bsparrow435 commented Jun 2, 2014

cmeiklejohn commented Jun 2, 2014

bsparrow435 commented Jun 2, 2014

cmeiklejohn commented Jun 2, 2014

cmeiklejohn commented Jun 4, 2014

cmeiklejohn Jun 4, 2014

bsparrow435 Jun 4, 2014

bsparrow435 commented Jun 4, 2014

cmeiklejohn Jun 4, 2014

bsparrow435 Jun 4, 2014

bsparrow435 Jun 4, 2014

cmeiklejohn Jun 4, 2014

reiddraper Jun 5, 2014

bsparrow435 Jun 5, 2014

bsparrow435 Jun 5, 2014

cmeiklejohn Jun 5, 2014

reiddraper Jun 5, 2014

cmeiklejohn Jun 5, 2014

cmeiklejohn commented Jun 4, 2014

bsparrow435 commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

Add logic to clear cancelled connection requests #598

Add logic to clear cancelled connection requests #598

Conversation

bsparrow435 commented Jun 2, 2014

cmeiklejohn commented Jun 2, 2014

bsparrow435 commented Jun 2, 2014

cmeiklejohn commented Jun 2, 2014

cmeiklejohn commented Jun 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsparrow435 commented Jun 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmeiklejohn commented Jun 4, 2014

bsparrow435 commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

bsparrow435 commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014

cmeiklejohn commented Jun 5, 2014