-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Mock Transport: Allow to simulate network failures #5631
Conversation
An infrastructure that allows to simulate different network topologies failures, including 2 basic ones in failure to send requests, and unresponsive nodes
@@ -186,6 +187,7 @@ public TestCluster(long clusterSeed, int minNumNodes, int maxNumNodes, String cl | |||
builder.put("path.data", dataPath.toString()); | |||
} | |||
} | |||
builder.put(TransportModule.TRANSPORT_SERVICE_TYPE_KEY, MockTransportService.class.getName()); |
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.
should this go into the section where do the other modules in private static Settings getRandomNodeSettings(long seed)
there is a if (ENABLE_MOCK_MODULES
where this should go?
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.
will move it to the if statement in the ENABLE_MOCK code part, if tests will need it, then they should explicitly set it.
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.
+1 other than than LGTM
An infrastructure that allows to simulate different network topologies failures, including 2 basic ones in failure to send requests, and unresponsive nodes closes #5631
* is added to fail as well. | ||
*/ | ||
public void addFailToSendNoConnectRule(DiscoveryNode node) { | ||
((LookupTestTransport) transport).transports.put(node, new DelegateTransport(transport) { |
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.
Instead of passing down transport
in DelegateTransport, I think we should pass in there the actual transport used (local or netty). Right now a stack overflow can happen if LookupTestTransport#getTransport(DiscoveryNode) has a miss in its transports
map.
If we keep track of the actual Transport used in this class by saving it in a field which the constructor sets and we use that one instead then the stackoverflow can't happen.
Left a comment other then that it looks good. Maybe we should also have a few helper methods in TestCluster that delegate to MockTransportService? For example: public void addFailToSendNoConnectRule(String fromNode, String toNode) {
....
}
public void clearNoConnectRule(String fromNode, String toNode) {
...
} Internally these methods would fetch the MockTransportService from the right node and translate the node name into the DiscoveryNode. This would be nice since it should reduce some boiler plate code. |
An infrastructure that allows to simulate different network topologies failures, including 2 basic ones in failure to send requests, and unresponsive nodes