Autorecovering connections should keep endpoint-related fields up-to-date during recovery

[EGlaznev]

This is some kind of usability in source code :)
When performed
fh = m_factory.CreateFrameHandler(m_factory.Endpoint.CloneWithHostname(h));
reachableHostname = h;
a member m_factory.Endpoint remains untoched and contains useless default value 'localhost:5672'.
And even when a connection for server from 'hostnames' is establied - m_factory.Endpoint still remains default. But if we have only the 1 active connection - let's store it in m_factory.Endpoint :)?

The second part: instead of store valid host in 'reachableHostname' - I propose to store it in HostName :). This allows to change code some kind back - you do not need any more :) CloneWithHostname() and corresponding calls/fixes, because (for example) parameterless CreateFrameHandler() uses member Endpoint already (as far as I realized - exactly this was a reason for https://github.com/rabbitmq/rabbitmq-dotnet-client/issues/153, because as mentioned above - Endpoint.Clone... do not changes the member itself).

Something 'reverse-similar' was done here:
public virtual IConnection CreateConnection()
{
return CreateConnection(new List() { HostName });
}
It's cool when HostName is set - all next calls provide good results. But when user uses (oops) a direct call of CreateConnection(IList hostnames) - he didn't get cool 'live' Endpoint. After mine proposal he will get a fantastic feature - the name of really connected host from a whole list :)

Best regards,
Eugene.

[michaelklishin]

Feel free to submit a PR against the stable branch.

On 3 mar 2016, at 13:25, Eugene Glaznev notifications@github.com wrote:

This is some kind of usability in source code :)
When performed
fh = m_factory.CreateFrameHandler(m_factory.Endpoint.CloneWithHostname(h));
reachableHostname = h;
a member m_factory.Endpoint remains untoched and contains useless default value 'localhost:5672'.
And even when a connection for server from 'hostnames' is establied - m_factory.Endpoint still remains default. But if we have only the 1 active connection - let's store it in m_factory.Endpoint :)?

The second part: instead of store valid host in 'reachableHostname' - I propose to store it in HostName :). This allows to change code some kind back - you do not need any more :) CloneWithHostname() and corresponding calls/fixes, because (for example) parameterless CreateFrameHandler() uses member Endpoint already (as far as I realized - exactly this was a reason for #153, because as mentioned above - Endpoint.Clone... do not changes the member itself).

Something 'reverse-similar' was done here:
public virtual IConnection CreateConnection()
{
return CreateConnection(new List() { HostName });
}
It's cool when HostName is set - all next calls provide good results. But when user uses (oops) a direct call of CreateConnection(IList hostnames) - he didn't get cool 'live' Endpoint. After mine proposal he will get a fantastic feature - the name of really connected host from a whole list :)

Best regards,
Eugene.

—
Reply to this email directly or view it on GitHub.

[michaelklishin]

I'm sorry but I can't really make sense of what you're asking for, for example, what is a "cool 'live' Engpoint"? "reverse-similar"? Why is this feature so "fantastic"? Can you please start by explaining what you expect to happen during recovery?

[EGlaznev]

Ok, let's try.
When Endpoint always remains 'localhost:5672' - it's 'dead' because it's
useless and do not reflect the current situation.
When this member will get a real value of currently connected host - it's
definitely 'live' = precisely reflecting the current state of connection.
And all this is true about the first connection too.

1. Let's assume we have 3 servers in List<> - a 'First:1234' is down, a
   'Second:5678' is available and was successfully connected right now, a
   'Third' is down too.
2. My proposal is to set HostName to 'Second', Port to number '5678' and
   Endpoint to 'amqp-0-9://Second:5678' - right after when the connection has
   been established.
3. And after network failure and auto-reconnection to another host from
   list - I think it's useful to set all corresponding variables to the new
   values, for example HostName='Third', Port=5672 and the corresponding
   Endpoint object.

This is what I've called a 'live' situation. It's fantastic :) because we
will know and can write in the log (for example) exactly which host is
connected at the moment.

2016-03-03 14:18 GMT+03:00 Michael Klishin notifications@github.com:

I'm sorry but I can't really make sense of what you're asking for, for
example, what is a "cool 'live' Engpoint"? "reverse-similar"? Why is this
feature so "fantastic"? Can you please start by explaining what you expect
to happen during recovery?

—
Reply to this email directly or view it on GitHub
#158 (comment)
.

SY,
Eugene Glaznev.

[michaelklishin]

OK, so some endpoint-related fields are not updated during recovery. Got it. Feel free to submit a PR.

[kjnilsson]

Hi @EGlaznev,

Been looking at this request. I hope I have understood it correctly. Apologies if not.
I am not convinced the factory object should be mutated based on in the Connection instances it generates. If you want current Endpoint information it may be better to inspect the Endpoint property on the Connection instance itself. This should be up to date at least for the case of auto-recovering connections.

Cheers
Karl

[kjnilsson]

I do think there is a valid bug with the hostnames collection being ignored if autorecovery is not enabled. I have raised this as a separate bug #176

[kjnilsson]

Hi @EGlaznev I'm going to close this issue as you can get the live Endpoint info from the connection instead of the ConnectionFactory. Please re-open or raise another issue if we haven't analysed the request correctly.
Cheers
Karl