Add profiles to Netty transport infos #9134
Conversation
| * Further profile bound addresses | ||
| * @return Should return null if transport does not support profiles, otherwise a map with name of profile and its bound transport address | ||
| */ | ||
| Map<String, BoundTransportAddress> profileBoundAddresses(); |
There was a problem hiding this comment.
does this make this issue a breaking one? I know, always the same dicussion but I never remember the outcome, it potentially breaks plugins that implement their own transport...
There was a problem hiding this comment.
I'd say, it is breaking as well and we should mention it at least
|
Left a few minor comments |
spinscale
force-pushed
the
1501-expose-netty-profile-infos
branch
from
8ea6fcd
to
aca0071
Compare
| throw new BindTransportException("Failed to resolve publish address", e); | ||
| } | ||
| profileBoundAddresses.put(name, new BoundTransportAddress(new InetSocketTransportAddress(boundAddress), new InetSocketTransportAddress(publishAddress))); | ||
| } |
There was a problem hiding this comment.
I got confused at first about this new block of code. It adds ability to configure publish host and port per profile, something that was missing before. I wonder if this change deserves its own specific PR, otherwise let's just update the description of this PR to clarify that this change is made too. Also, I wonder if we can reuse code that seems the same in doStart
There was a problem hiding this comment.
true, updated commit message and refactored out an own method
spinscale
force-pushed
the
1501-expose-netty-profile-infos
branch
from
aca0071
to
f9040de
Compare
| int publishPort = settings.getAsInt("publish_port", boundAddress.getPort()); | ||
| String publishHost = settings.get("publish_host", this.settings.get("transport.publish_host", this.settings.get("transport.host"))); | ||
| InetSocketAddress publishAddress = createPublishAddress(publishHost, publishPort); | ||
| profileBoundAddresses.put(name, new BoundTransportAddress(new InetSocketTransportAddress(boundAddress), new InetSocketTransportAddress(publishAddress))); |
There was a problem hiding this comment.
seems like some more code could be shared with the above block, unless I am missing something :)
Also why do we only read publish_port here and not the other transport.publish_port etc. ?
There was a problem hiding this comment.
changed the code slightly, but now it is too different to actually merge it (or I am not seeing right now)
There was a problem hiding this comment.
Ok, then for the profile settings you only read the publish_port and publish_host instead of the other settings. I see the difference now. Before it seemd like you wanted to read exactly the same settings, then I wondered why that part couldn't be shared.
spinscale
force-pushed
the
1501-expose-netty-profile-infos
branch
from
f9040de
to
28d774c
Compare
spinscale
force-pushed
the
1501-expose-netty-profile-infos
branch
from
28d774c
to
4bfdc0b
Compare
|
|
||
| TransportInfo() { | ||
| } | ||
|
|
||
| public TransportInfo(BoundTransportAddress address) { | ||
| public TransportInfo(BoundTransportAddress address, Map<String, BoundTransportAddress> profileAddresses) { |
There was a problem hiding this comment.
Should mark profileAddresses as @Nullable here perhaps?
|
@spinscale left some comments, I think we should always output the |
|
Also, I'm not sure why everything in |
|
LGTM with the comments made on the review |
spinscale
force-pushed
the
1501-expose-netty-profile-infos
branch
from
4bfdc0b
to
944548d
Compare
|
incorporated review comments |
|
LGTM |
Until now, there was no possibility to expose infos about configured transport profiles. This commit adds the ability to expose those information in the TransportInfo class. The channel was well as the netty pipeline handler now also contain the profile they were configured for, as this information cannot be extracted elsewhere. In addition, each profile now can set its own publish host and port, which might be needed in case of portforwarding or using docker. Closes elastic#9134
spinscale
force-pushed
the
1501-expose-netty-profile-infos
branch
from
944548d
to
59f8c09
Compare
Until now, there was no possibility to expose infos about configured transport profiles. This commit adds the ability to expose those information in the TransportInfo class. The channel was well as the netty pipeline handler now also contain the profile they were configured for, as this information cannot be extracted elsewhere. In addition, each profile now can set its own publish host and port, which might be needed in case of portforwarding or using docker. Closes #9134
clintongormley
added
:Distributed/Network
clintongormley
changed the title
Netty Transport: Add profiles to transport infos
Until now, there was no possibility to expose infos about configured
transport profiles. This commit adds the ability to expose those
information in the TransportInfo class.
The channel was well as the netty pipeline handler now also contain
the profile they were configured for, as this information cannot be
extracted elsewhere.
In addition, each profile now can set its own publish host and port,
which might be needed in case of portforwarding or using docker.