Rfc6724 resolver ordering semantics#778
Conversation
arturobernalg
force-pushed
the
Rfc6724A
branch
from
0802ffe to
3a1bd12
Compare
rschmitt
left a comment
rschmitt
left a comment
There was a problem hiding this comment.
Main thing here is that I'd like to see a proper implementation of INTERLEAVE, more unit test coverage, and an example program we can use for manual testing.
...client5/src/test/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolverTest.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolver.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/AddressSelectingDnsResolver.java
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolver.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/AddressSelectingDnsResolver.java
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/AddressSelectingDnsResolver.java
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/config/ProtocolFamilyPreference.java
Show resolved
Hide resolved
| InetAddress src = null; | ||
| try (final DatagramSocket s = new DatagramSocket()) { | ||
| s.connect(dest); // does not send packets; OS picks source addr/if | ||
| src = s.getLocalAddress(); |
There was a problem hiding this comment.
You probably want to inject a thing here that you can mock for unit testing purposes, e.g. a Function<InetSocketAddress, InetAddress> (or an equivalent that throws IOException).
...client5/src/test/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolverTest.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolver.java
Outdated
Show resolved
Hide resolved
|
@rschmitt Please do another pass. I think i solve all your remarks |
httpclient5-testing/src/test/java/org/apache/hc/client5/testing/ManualRfc6724ResolverIT.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolver.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/AddressSelectingDnsResolver.java
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolver.java
Outdated
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/AddressSelectingDnsResolver.java
Show resolved
Hide resolved
httpclient5/src/main/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolver.java
Outdated
Show resolved
Hide resolved
| import org.apache.hc.client5.http.SystemDefaultDnsResolver; | ||
| import org.apache.hc.client5.http.config.ProtocolFamilyPreference; | ||
|
|
||
| public final class Rfc6724ResolverExample { |
There was a problem hiding this comment.
src/hc/client # ./run-example.sh Rfc6724ResolverExample www.amazon.com
Host: www.amazon.com
Preference: DEFAULT
IPv6 2600:1409:9800:1680:0:0:0:3bd4
IPv6 2600:1409:9800:168d:0:0:0:3bd4
IPv4 184.27.192.135
src/hc/client # ./run-example.sh Rfc6724ResolverExample www.amazon.com INTERLEAVE
Host: www.amazon.com
Preference: INTERLEAVE
IPv6 2600:1409:9800:168d:0:0:0:3bd4
IPv4 184.27.192.135
IPv6 2600:1409:9800:1680:0:0:0:3bd4
Neat! I'd be cool if I could see the before-and-after (maybe you could intercept the "before" by wrapping the system default resolver, then passing the wrapper in to the RFC 6724 resolver), but it's not essential.
...client5/src/test/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolverTest.java
Outdated
Show resolved
Hide resolved
...client5/src/test/java/org/apache/hc/client5/http/Rfc6724AddressSelectingDnsResolverTest.java
Outdated
Show resolved
Hide resolved
httpclient5-testing/src/test/java/org/apache/hc/client5/testing/ManualRfc6724ResolverIT.java
Outdated
Show resolved
Hide resolved
|
There are some things from my first review that still haven't been addressed. I suggest going through the comments one by one and hitting "Resolve conversation" as you address each one, so you can track what is and isn't done. |
Hi @rschmitt I believe I’ve addressed all your remarks in the latest update. |
Define INTERLEAVE as no-bias and align tests and debug output.
Add manual gated IT to dump DEFAULT vs INTERLEAVE results. Expand unit coverage for scope mapping and core RFC comparison rules.
…ults. Tighten resolver semantics and filtering for HTTP/TCP connect targets.
arturobernalg
force-pushed
the
Rfc6724A
branch
from
d3d8e07 to
4892d57
Compare
2 participants
Introduce Rfc6724AddressSelectingDnsResolver and unit tests.
Define INTERLEAVE as “no bias” (preserve RFC6724 order); keep ONLY/PREFER behavior unchanged.