New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose all addresses that the transports are bound to #13586

Merged
merged 1 commit into from Sep 23, 2015

Conversation

Projects
None yet
3 participants
@jaymode
Member

jaymode commented Sep 15, 2015

In #12942, the NettyTransport and NettyHttpServerTransport were updated to allow for binding
to multiple addresses. However, the BoundTransportAddress holder only exposed the first address
that the transport was bound to and this object is used to populate the values returned to the user
via our APIs.

This change exposes all of the bound addresses in the BoundTransportAddress holder, which allows
for an accurate representation of all interfaces that elasticsearch is bound to and listening on.

@clintongormley clintongormley changed the title from expose all addresses that the transports are bound to to Expose all addresses that the transports are bound to Sep 18, 2015

out.writeInt(boundAddresses.length);
for (TransportAddress address : boundAddresses) {
TransportAddressSerializers.addressToStream(out, address);
}
TransportAddressSerializers.addressToStream(out, publishAddress);
}
@Override
public String toString() {

This comment has been minimized.

@rmuir

rmuir Sep 21, 2015

Contributor

Is this the piece that gets logged at INFO level on startup? This would fix the confusing printout we do today.

So we may want to lower the per-address bind printout that we do at INFO level in Netty/HTTP, make it DEBUG or something, because we will be printing them all now at the end?

@rmuir

rmuir Sep 21, 2015

Contributor

Is this the piece that gets logged at INFO level on startup? This would fix the confusing printout we do today.

So we may want to lower the per-address bind printout that we do at INFO level in Netty/HTTP, make it DEBUG or something, because we will be printing them all now at the end?

This comment has been minimized.

@jaymode

jaymode Sep 21, 2015

Member

Yeah this is what gets printed out. This is the current output:

[2015-09-21 13:42:27,541][INFO ][org.elasticsearch.http.netty] [Pyro] Bound http to address {127.0.0.1:9200}
[2015-09-21 13:42:27,542][INFO ][org.elasticsearch.http.netty] [Pyro] Bound http to address {[fe80::1]:9200}
[2015-09-21 13:42:27,543][INFO ][org.elasticsearch.http.netty] [Pyro] Bound http to address {[::1]:9200}
[2015-09-21 13:42:27,544][INFO ][org.elasticsearch.http   ] [Pyro] publish_address {127.0.0.1:9200}, bound_addresses {127.0.0.1:9200}, {[fe80::1]:9200}, {[::1]:9200}

I'll make the INFO level per address messages DEBUG.

@jaymode

jaymode Sep 21, 2015

Member

Yeah this is what gets printed out. This is the current output:

[2015-09-21 13:42:27,541][INFO ][org.elasticsearch.http.netty] [Pyro] Bound http to address {127.0.0.1:9200}
[2015-09-21 13:42:27,542][INFO ][org.elasticsearch.http.netty] [Pyro] Bound http to address {[fe80::1]:9200}
[2015-09-21 13:42:27,543][INFO ][org.elasticsearch.http.netty] [Pyro] Bound http to address {[::1]:9200}
[2015-09-21 13:42:27,544][INFO ][org.elasticsearch.http   ] [Pyro] publish_address {127.0.0.1:9200}, bound_addresses {127.0.0.1:9200}, {[fe80::1]:9200}, {[::1]:9200}

I'll make the INFO level per address messages DEBUG.

This comment has been minimized.

@rmuir

rmuir Sep 21, 2015

Contributor

Nice, much better. This removes a ton of confusion at startup, it just was not possible to easily fix without fixing this holder class the way you did.

@rmuir

rmuir Sep 21, 2015

Contributor

Nice, much better. This removes a ton of confusion at startup, it just was not possible to easily fix without fixing this holder class the way you did.

@rmuir

View changes

Show outdated Hide outdated .../java/org/elasticsearch/common/transport/BoundTransportAddressTests.java
@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Sep 21, 2015

Contributor

this looks good to me. I added some minor suggestions. My only other wish, would be to add some heavy comments around the big conditionals with DEFAULT_PROFILE in the transport binding. Some of it is pre-existing conditions, but its hard to wrap your head around, and seeing the null checks etc is not intuitive towards figuring it out. I know i struggled with this part when first looking at this code, then figured it out for about 30 minutes (enough to make my changes) and promptly forgot it.

Contributor

rmuir commented Sep 21, 2015

this looks good to me. I added some minor suggestions. My only other wish, would be to add some heavy comments around the big conditionals with DEFAULT_PROFILE in the transport binding. Some of it is pre-existing conditions, but its hard to wrap your head around, and seeing the null checks etc is not intuitive towards figuring it out. I know i struggled with this part when first looking at this code, then figured it out for about 30 minutes (enough to make my changes) and promptly forgot it.

@rmuir

This comment has been minimized.

Show comment
Hide comment
@rmuir

rmuir Sep 21, 2015

Contributor

Looks great, thanks for fixing this!

Contributor

rmuir commented Sep 21, 2015

Looks great, thanks for fixing this!

expose all addresses that the transports are bound to
In #12942, the NettyTransport and NettyHttpServerTransport were updated to allow for binding
to multiple addresses. However, the BoundTransportAddress holder only exposed the first address
that the transport was bound to and this object is used to populate the values returned to the user
via our APIs.

This change exposes all of the bound addresses in the BoundTransportAddress holder, which allows
for an accurate representation of all interfaces that elasticsearch is bound to and listening on.

@jaymode jaymode merged commit 5b8b15e into elastic:master Sep 23, 2015

1 check passed

CLA Commit author has signed the CLA
Details

@clintongormley clintongormley added v2.0.0-rc1 and removed v2.0.0 labels Oct 7, 2015

@clintongormley clintongormley removed the v2.1.0 label Nov 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment