Hi Rajini,
Good point, `getHostString()` actually returns the original host. The
InetSocketAddress code is a bit confusing as it does:
String host = null;
try {
addr = InetAddress.getByName(hostname);
} catch(UnknownHostException e) {
host = hostname;
}
holder = new InetSocketAddressHolder(host, addr, checkPort(port));
However, the the holder keeps track of the original address too. So, the
change proposed in the KIP should fix the bootstrap behavior too. I still
think the code would be easier to reason about if we returned a List<Node>
instead of List<InetSocketAddress>. But that change can be done
independently of the KIP.
Ismael
On Tue, May 26, 2020 at 1:26 AM Rajini Sivaram <rajinisiva...@gmail.com>
wrote:
> Hi Ismael,
>
> I think we resolve early to parse and verify bootstrap servers and use
> InetSocketAddress as a convenient way to propagate host:port. We also use
> the opportunity to handle `resolve_canonical_bootstrap_servers_only` since
> that is applied only to bootstrap servers. In other cases, even though we
> create an InetSocketAddress and that resolves to a single address, we don't
> use the single resolved address. We create Node instances using
> InetSocketAddress.getHostString(), so that gives us the original bootstrap
> host. And that should ensure we use all IPs for use_all_dns_ips. Have I
> missed something?
>
> Regards,
>
> Rajini
>
> On Tue, May 26, 2020 at 6:51 AM Ismael Juma <ism...@juma.me.uk> wrote:
>
> > Badai,
> >
> > I was looking at the code and it seems like we should change how the
> > bootstrap resolution is done as part of this KIP. The way it works at the
> > moment follows:
> >
> > 1. We create a single InetSocketAddress by calling the constructor with
> the
> > host and port. That invokes InetAddress.getByName(host), which takes the
> > first result of `getAllByName`.
> > 2. Using the consumer as an example, it takes the list with one
> > InetSocketAddress and passes it to ConsumerMetadata.bootstrap(), which
> > creates a node via `new Node(nodeId, address.getHostString(),
> > address.getPort()))`. If the first address happens to be down, we don't
> try
> > any other address.
> >
> > I think this goes against the goals of this KIP, which is to use any of
> the
> > available addresses in every case (including bootstrap). It's not clear
> to
> > me why we try to resolve the address in the first place in the default
> > case. Instead, we should create `Node` with the original host and port.
> > When we try to connect, the existing logic for iterating over the
> existing
> > addresses will be used.
> >
> > Rajini, any reason why this would not work?
> >
> > Ismael
> >
> > On Mon, May 25, 2020 at 5:03 PM Badai Aqrandista <ba...@confluent.io>
> > wrote:
> >
> > > The vote for KIP-602 has passed with 5 binding and 1 non-binding +1s,
> and
> > > no objections.
> > >
> > > Thanks everyone for reviews and feedback,
> > >
> > > Badai
> > >
> > > On Mon, May 25, 2020 at 12:21 PM Gwen Shapira <g...@confluent.io>
> wrote:
> > >
> > > > +1 (binding)
> > > >
> > > > Thank you!
> > > >
> > > > On Fri, May 22, 2020 at 1:40 AM Badai Aqrandista <ba...@confluent.io
> >
> > > > wrote:
> > > >
> > > > > Hi All,
> > > > >
> > > > > I would like to start the vote on KIP-602: Change default value for
> > > > > client.dns.lookup
> > > > >
> > > > > For reference, here is the KIP wiki:
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-602%3A+Change+default+value+for+client.dns.lookup
> > > > >
> > > > > And discussion thread:
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> https://lists.apache.org/thread.html/r0e70d3757267c4158f12c05a4e5ac9eb33f2d11ce99d5878b3b4b3f7%40%3Cdev.kafka.apache.org%3E
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Badai
> > > > >
> > > >
> > > >
> > > > --
> > > > Gwen Shapira
> > > > Engineering Manager | Confluent
> > > > 650.450.2760 | @gwenshap
> > > > Follow us: Twitter | blog
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > Badai
> > >
> >
>
