-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Zen2: Add basic Zen1 transport-level BWC #35443
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
Conversation
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few nits. It looks like we're missing handlers for internal:discovery/zen/rejoin
and internal:discovery/zen/leave
. Not sure we need to make them do anything interesting here yet, but it'd be good to have placeholders so they don't get forgotten.
} | ||
} | ||
|
||
public static Settings.Builder addZen1Attribute(Settings.Builder builder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps insert:
// only here temporarily for BWC development, TODO remove once complete
Assuming we are going to remove this before GA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in 9b12b05
} | ||
|
||
public static boolean isZen1Node(DiscoveryNode discoveryNode) { | ||
return discoveryNode.getVersion().before(Version.V_7_0_0) || discoveryNode.getAttributes().containsKey("zen1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, if the zen1
attribute is going to go away then we should note this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in 9b12b05
@@ -103,8 +108,12 @@ public FollowersChecker(Settings settings, TransportService transportService, | |||
followerCheckRetryCount = FOLLOWER_CHECK_RETRY_COUNT_SETTING.get(settings); | |||
|
|||
updateFastResponseState(0, Mode.CANDIDATE); | |||
transportService.registerRequestHandler(FOLLOWER_CHECK_ACTION_NAME, Names.SAME, FollowerCheckRequest::new, | |||
transportService.registerRequestHandler(FOLLOWER_CHECK_ACTION_NAME, Names.SAME, false, false, FollowerCheckRequest::new, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell this change is to set canTripCircuitBreaker
to false. Do we definitely want to count a follower as healthy if a ping would trip its circuit breaker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. I wonder if it's something we should discuss with a larger group, but go with Zen1 defaults for now.
if (Coordinator.isZen1Node(discoveryNode)) { | ||
actionName = NodesFaultDetection.PING_ACTION_NAME; | ||
transportRequest = new NodesFaultDetection.PingRequest(discoveryNode, ClusterName.CLUSTER_NAME_SETTING.get(settings), | ||
transportService.getLocalNode(), 0L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ClusterState.UNKNOWN_VERSION
instead of 0L
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in cf3d83e
if (Coordinator.isZen1Node(discoveryNode)) { | ||
actionName = UnicastZenPing.ACTION_NAME; | ||
transportRequest = new UnicastZenPing.UnicastPingRequest(1, ZenDiscovery.PING_TIMEOUT_SETTING.get(settings), | ||
new ZenPing.PingResponse(getLocalNode(), null, ClusterName.CLUSTER_NAME_SETTING.get(settings), 0L)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ClusterState.UNKNOWN_VERSION
instead of 0L
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in cf3d83e
final List<ZenPing.PingResponse> pingResponses = new ArrayList<>(); | ||
final ClusterName clusterName = ClusterName.CLUSTER_NAME_SETTING.get(settings); | ||
pingResponses.add(new ZenPing.PingResponse(transportService.getLocalNode(), peersResponse.getMasterNode().orElse(null), | ||
clusterName, 0L)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ClusterState.UNKNOWN_VERSION
instead of 0L
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in cf3d83e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implements serialization compatibility between Zen1 and Zen2 transport action, allowing a Zen1 node join a Zen2 master and vice-versa.