@arthurschreiber This takes care of the remaining issues with instance-lookup for multi-subnet failover (#362).

Please take a look.

@tvrprasad Thanks for preparing this. ❤️

I did look through the changes, and I'm a bit confused now. As UDP is connectionless, I don't believe there is a need for the different connection strategies you propose here.

You only need two sockets, one for UDPv4 and one for UDPv6. All Server Browser requests can reuse these two sockets, there is no need to create a new socket for every message to be sent.

All requests can be send off in parallel, as the response coming back should be the same. Whenever the first response comes back, we can close both sockets and be done.

I do definitely think that some clean up of the InstanceListenercode would be in order, but the changes here seem to go a bit too far. 😧

I'll try to come up with a more actionable feedback tomorrow.

@arthurschreiber All good points. I'll take a crack at addressing them.

@arthurschreiber After thinking about this a bit more, I'm wondering if two send strategies, parallel and sequential, might still make sense - for the same reason we have two strategies for TCP. We have SequentialConnectionStrategy presumably to save network and machine resources by not setting up multiple connections when the user does not specify MutliSubnetFailover. I think the same logic should apply to UDP case also, as we will use some machine and network resources by doing multiple sends in parallel. Seems consistent. Thoughts?

Still agreed on needing only two sockets at most. I'll put that change together while I wait to hear from you on the above. Thanks.

After thinking about this a bit more, I'm wondering if two send strategies, parallel and sequential, might still make sense - for the same reason we have two strategies for TCP.

In general, I'd agree, yes. Separation of concerns is "a good thing"™️. But in this case, you need to take into consideration that UDP is quite different from TCP/IP.

As UDP is connectionless, there is no overhead of establishing a connection, neither on the sending nor on the receiving side. There is also almost no associated overhead of sending a message, both the request as well as the reply are tiny, and they are fire and forget - there is no retrying or anything on the protocol level.

There is really no advantage of ever not sending all the server browser requests in parallel, and only processing the first response.

I hope these thoughts help you understand my position! 😄

Yes, I understand your position :-)

In that case, would it be reasonable to assume there is little overhead to creating Dgram sockets? The code is simpler and easier to test if I create a new socket for each IP address.

If that sounds ok, I can send an update to the PR that gets rid of the SequentialSendStrategy.

Never mind that comment about creating a socket for each IP address. I'm fixing that to create only two sockets. So here is the summary of changes I'm going to make:

1. Get rid of SequentialSendStrategy.
2. Use ParallelSendStrategy anytime hostname is specified irrespective of multiSubnetFailover value.
   • One option here is to not do lookupAll when multiSubnetFailover is false, and just do a send to the hostname. But I think it's more consistent with what we're doing on TCP side of things, and more reliable, if we do lookupAll and do a send to all IPs.
3. Create at most two sockets, one for v4 and one for v6, instead of one for each IP.

Please let me know if that doesn't sound right. Thanks!

@arthurschreiber Updated PR with all changes discussed. Please make a pass. Thanks!

@arthurschreiber Please let me know if there is anything blocking merging this. Happy to address any concerns.

@tvrprasad I've not forgotten about this yet. I promise to get to this (and the 1.15.0 release) sometime this week! 👍

Thanks @arthurschreiber !

@tvrprasad I found https://msdn.microsoft.com/en-us/library/cc219703.aspx, which describes the "SQL Server Resolution Protocol", which is what the instance lookup code is supposed to implement.

The tedious implementation deviates from the specification in a few places, so I started work on bringing us in line with the specification (which actually also simplifies the implementation). I'll open a PR shortly!

@arthurschreiber I looked at the specification and I see two places where we seem to deviate.

1. The client message we send needs to be CLNT_UCAST_INST (single byte 0x04).
   • We'll still want to send this message to all resolved IPs, I don't think that changes.
2. The server response parsing could be simplified since the response will now have a single instance. This is not really a deviation.

Is there anything else? If I got this right, I think this PR is orthogonal to your changes as this just adds support for IPv6 and makes the code more robust (I think :-)) by making it unit testable and adding tests. We should be able to merge this (after incorporating any feedback, of course) and commit your changes on top. Thoughts?

And thanks for digging into the spec!

One step closer to the release, yay!! :-)