Skip to content

Log leader and handshake failures by default #42342

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 30, 2019
Merged

Log leader and handshake failures by default #42342

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9366fb4
Log leader and handshake failures by default
DaveCTurner May 22, 2019
9c64b02
Merge branch 'master' into 2019-05-22-log-leader-and-handshake-failures
DaveCTurner May 28, 2019
bd0c29c
Pass leader failures onto coordinator
DaveCTurner May 28, 2019
ed6a2e2
Better message
DaveCTurner May 28, 2019
7013637
Fix tests
DaveCTurner May 28, 2019
7a32587
Merge branch 'master' into 2019-05-22-log-leader-and-handshake-failures
DaveCTurner May 29, 2019
f55db2a
Tidy up messages and exceptions
DaveCTurner May 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions server/src/main/java/org/elasticsearch/cluster/coordination/LeaderChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,15 +234,21 @@ public void handleException(TransportException exp) {
}

if (exp instanceof ConnectTransportException || exp.getCause() instanceof ConnectTransportException) {
logger.debug(new ParameterizedMessage("leader [{}] disconnected, failing immediately", leader), exp);
if (logger.isDebugEnabled()) {
logger.debug(new ParameterizedMessage(
"leader [{}] disconnected during check, restarting discovery", leader), exp);
} else {
logger.info("leader [{}] disconnected during check, restarting discovery", leader);
Copy link
Contributor

@ywelsch ywelsch May 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of logging here at info level, I would prefer to pass the necessary info (message + exception) to Coordinator.getOnLeaderFailure and let that one do the logging if it effectively turns the node back to candidate.
Also I would prefer not to use the "leader" terminology when logging at info level.

Copy link
Contributor Author

@DaveCTurner DaveCTurner May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've moved the logging to the Coordinator in bd0c29c.

}
leaderFailed();
return;
}

long failureCount = failureCountSinceLastSuccess.incrementAndGet();
if (failureCount >= leaderCheckRetryCount) {
logger.debug(new ParameterizedMessage("{} consecutive failures (limit [{}] is {}) so leader [{}] has failed",
failureCount, LEADER_CHECK_RETRY_COUNT_SETTING.getKey(), leaderCheckRetryCount, leader), exp);
logger.info(new ParameterizedMessage(
"leader [{}] has failed {} consecutive checks (limit [{}] is {}), restarting discovery; last failure was:",
leader, failureCount, LEADER_CHECK_RETRY_COUNT_SETTING.getKey(), leaderCheckRetryCount), exp);
leaderFailed();
return;
}
Expand All @@ -269,6 +275,7 @@ void leaderFailed() {

void handleDisconnectedNode(DiscoveryNode discoveryNode) {
if (discoveryNode.equals(leader)) {
logger.info("leader [{}] disconnected, restarting discovery", leader);
leaderFailed();
}
}
Expand Down
8 changes: 8 additions & 0 deletions server/src/main/java/org/elasticsearch/discovery/HandshakingTransportAddressConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.cluster.node.DiscoveryNode;
Expand Down Expand Up @@ -89,6 +90,13 @@ protected void doRun() throws Exception {
remoteNode = transportService.handshake(connection, probeHandshakeTimeout.millis());
// success means (amongst other things) that the cluster names match
logger.trace("[{}] handshake successful: {}", this, remoteNode);
} catch (Exception e) {
// we opened a connection and successfully performed a low-level handshake, so we were definitely talking to an
// Elasticsearch node, but the high-level handshake failed indicating some kind of mismatched configurations
// (e.g. cluster name) that the user should address
logger.warn(new ParameterizedMessage("handshake failed for [{}]", this), e);
Copy link
Contributor

@ywelsch ywelsch May 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do this only for the handshake bit? Can we avoid logging if it is truly a connection failure? e.g. ConnectTransportException.

Copy link
Contributor Author

@DaveCTurner DaveCTurner May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. As the comment says, we know that we successfully connected and performed a low-level handshake, so even a ConnectTransportException here seems worthy of a warning.

Copy link
Contributor

@ywelsch ywelsch May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid for a flaky network connection to cause extra verbose warnings here. I'm fine if you want to leave it this way though

listener.onFailure(e);
return;
} finally {
IOUtils.closeWhileHandlingException(connection);
}
Expand Down