Log warning if initiateChannel fails

[DaveCTurner]

Also account for the fact that channelFuture.cause() might be null.

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[DaveCTurner]

Note that mostly we do already log a failure here much further up the stack, for instance:

elasticsearch/server/src/main/java/org/elasticsearch/discovery/HandshakingTransportAddressConnector.java

Lines 187 to 199 in f3a1664

	logger.warn(
	() -> format(
	"""
	Successfully discovered master-eligible node [%s] at address [%s] but could not connect to it at its \
	publish address of [%s]. Each node in a cluster must be accessible at its publish address by all other \
	nodes in the cluster. See %s for more information.""",
	remoteNode.descriptionWithoutAttributes(),
	transportAddress,
	remoteNode.getAddress(),
	ReferenceDocs.NETWORK_BINDING_AND_PUBLISHING
	),
	e
	);

elasticsearch/server/src/main/java/org/elasticsearch/cluster/coordination/Coordinator.java

Lines 675 to 681 in cedcb5c

	logger.warn(
	() -> format(
	"received join request from [%s] but could not connect back to the joining node",
	joinRequest.getSourceNode()
	),
	e
	);

elasticsearch/server/src/main/java/org/elasticsearch/cluster/NodeConnectionsService.java

Lines 340 to 344 in f1e3058

	// Only warn every 6th failure. We work around this log while stopping integ test clusters in InternalTestCluster#close
	// by temporarily raising the log level to ERROR. If the nature of this log changes in the future, that workaround might
	// need to be adjusted.
	final Level level = currentFailureCount % 6 == 1 ? Level.WARN : Level.DEBUG;
	logger.log(level, () -> format("failed to connect to %s (tried [%s] times)", discoveryNode, currentFailureCount), e);

However it does seem unusual enough to deserve its own log message every time too.

[mhl-b]

I believe you need a callback. Checking null on channel might be not enough.
connectFuture.addListener(f -> if (f.isSuccess() == false) { log.error...})

[DaveCTurner]

We add that listener here:

elasticsearch/modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/Netty4TcpChannel.java

Line 47 in 3504c27

addListener(connectFuture, connectContext);

I don't think we want to log every such failure, and definitely not at error. The logging we have today is enough for that.

[DaveCTurner]

Wait sorry I get what you mean: if channel == null we should still wait for connectFuture to complete before logging the error.

[mhl-b]

Ah, I see. addListener(connectFuture, connectContext); should be enough. if (channel == null) block is confusing, channel is undefined until future is resolved. We should pass only connectFuture, not channel, to the Netty4TcpChannel, once connectFuture is resolved Netty4TcpChannel should update it's own channel.

[mhl-b]

channel is undefined

I meant there are few steps that can go wrong - channel initialization and registration and failures are dispatched at different thread either event-loop or global-executor. In all cases channel would be closed forcibly. But using channel that failed to initialize means we don't have our handlers attached.