scylladb / java-driver

ScyllaDB Java Driver for ScyllaDB and Apache Cassandra, based on the DataStax Java Driver
Apache License 2.0
63 stars 37 forks source link

4.x: drop `ResolverProviderTest` in favor of `org.burningwave.tools.net.HostResolutionRequestInterceptor` #351

Closed dkropachev closed 1 month ago

dkropachev commented 1 month ago

At https://github.com/scylladb/java-driver/pull/332 we have introduced a ResolverProvider to scylla-4.x that allows us to hook to dns resolution logic in the driver.

At https://github.com/scylladb/java-driver/pull/344 @Bouncheck found better solution (org.burningwave.tools.net.HostResolutionRequestInterceptor) to do that we exact same limitations, and it does not need hooking to driver code to achieve that.

So, let's drop ResolverProvider on scylla-4.x and use org.burningwave instead, all the tests stays in the code. It worth to mention that org.burningwave.tools.net.HostResolutionRequestInterceptor works just fine on java-11, so it is a "future proof".

Bouncheck commented 1 month ago

I am not sure if we can do the mapping of single hostname to multiple IPs with that

dkropachev commented 1 month ago

I am not sure if we can do the mapping of single hostname to multiple IPs with that

We 100% need that, can you double check on that ?

Bouncheck commented 1 month ago

The MappedHostResolver seems to use a normal Map and assume that single hostname can be mapped to single ip. Multiple hostnames can point to the same ip.

public class MappedHostResolver implements HostResolver {
  protected Map<String, String> hostAliases;

  ...

public Collection<InetAddress> getAllAddressesForHostName(Map<String, Object> argumentMap) {
    String hostName = (String)this.getMethodArguments(argumentMap)[0];
    Collection<InetAddress> addresses = new ArrayList();
    String iPAddress = (String)this.hostAliases.get(hostName);
    if (iPAddress != null) {
      try {
        addresses.add(InetAddress.getByAddress(hostName, IPAddressUtil.INSTANCE.textToNumericFormat(iPAddress)));
      } catch (UnknownHostException var6) {
      }
    }

    return addresses;
  }

  ...

However the return type here and interface signatures suggest that it should be possible to implement another version of HostResolver that would use a multimap (or map that accepts a list of addresses as a value)

dkropachev commented 1 month ago

The MappedHostResolver seems to use a normal Map and assume that single hostname can be mapped to single ip. Multiple hostnames can point to the same ip.

public class MappedHostResolver implements HostResolver {
  protected Map<String, String> hostAliases;

  ...

public Collection<InetAddress> getAllAddressesForHostName(Map<String, Object> argumentMap) {
    String hostName = (String)this.getMethodArguments(argumentMap)[0];
    Collection<InetAddress> addresses = new ArrayList();
    String iPAddress = (String)this.hostAliases.get(hostName);
    if (iPAddress != null) {
      try {
        addresses.add(InetAddress.getByAddress(hostName, IPAddressUtil.INSTANCE.textToNumericFormat(iPAddress)));
      } catch (UnknownHostException var6) {
      }
    }

    return addresses;
  }

  ...

However the return type here and interface signatures suggest that it should be possible to implement another version of HostResolver that would use a multimap (or map that accepts a list of addresses as a value)

@Bouncheck , can you please check if your suggestion true or not.

Bouncheck commented 1 month ago

I'll make a quick proof of concept

Bouncheck commented 1 month ago

It's working on a simple example.

SecondMappedResolver resolver = new SecondMappedResolver(new LinkedHashMap<>());
resolver.putHost("test.hostname", "127.1.1.1");
resolver.putHost("test.hostname", "127.1.1.2");
resolver.putHost("test.hostname", "127.1.1.3");
HostResolutionRequestInterceptor.INSTANCE.install(resolver, DefaultHostResolver.INSTANCE);

...

try {
      System.out.println("InetAddress.getByName(test.hostname) == " + InetAddress.getByName("test.hostname"));
    } catch (UnknownHostException e) {
      throw new RuntimeException(e);
    }

    try {
      System.out.println("InetAddress.getAllByName(test.hostname) == " + Arrays.toString(InetAddress.getAllByName("test.hostname")));
    } catch (UnknownHostException e) {
      throw new RuntimeException(e);
    }    

Produces the following output

InetAddress.getByName(test.hostname) == test.hostname/127.1.1.1
InetAddress.getAllByName(test.hostname) == [test.hostname/127.1.1.1, test.hostname/127.1.1.2, test.hostname/127.1.1.3]
dkropachev commented 1 month ago

@Bouncheck , great, can you please make s PR to move to org.burningwave.tools.net.HostResolutionRequestInterceptor

Bouncheck commented 1 month ago

Yes, already on it

Bouncheck commented 1 month ago

Created PR #354

dkropachev commented 1 month ago

Closed at https://github.com/scylladb/java-driver/pull/354.