Add MockResolverIT#replace_cluster_test()
#335
Conversation
integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java
Show resolved
Hide resolved
integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java
Outdated
Show resolved
Hide resolved
| getNodeInetAddress(ccmBridge, 2), | ||
| getNodeInetAddress(ccmBridge, 3) | ||
| })); | ||
| ResolverProvider.setDefaultResolverFactory(mockResolverFactory); |
There was a problem hiding this comment.
We need to make sure that there is only one instance of MockResolverFactory that is created and registered.
In the same way we do that for CCM_RULE.
I would recomment to have some method CreateAndRegisterMockResolverFactory that does that.
Also we need to move MockResolverFactory initialization to the class level.
There was a problem hiding this comment.
I'm not sure If I understand the intent here. Don't we want to have different test methods in this class that test different scenarios? So in that case we should be able to set different resolvers for each test method.
And for the scenario where we kill old cluster and start new with different IPs don't we want to have the ability to change the resolver to the new IPs too? I think this change would prevent that.
There was a problem hiding this comment.
I'm not sure If I understand the intent here. Don't we want to have different test methods in this class that test different scenarios? So in that case we should be able to set different resolvers for each test method.
Yes we want.
Not we don't need different resolvers, test can work with resolver targeting only dns records that are relevant to the test.
And for the scenario where we kill old cluster and start new with different IPs don't we want to have the ability to change the resolver to the new IPs too? I think this change would prevent that.
how so ? don't see anything that would block you from doing it.
ResolverProvider is a global factory, when you call ResolverProvider.setDefaultResolverFactory it changes resolverFactory globally.
Now, if you do that twice, second call will override first factory, but test won't be aware of that and fail, next you will have to debug such test to figure out what happend.
Not only that, if classes were already initialized with old resolver factory, you may get half of classes with old factory, half with new, which is going to create lot's of confusion.
To catch this issue early, we need ResolverFactory to throw an exception when this happnes.
There was a problem hiding this comment.
I believe that now it's as we discussed
integration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java
Show resolved
Hide resolved
Bouncheck
force-pushed
the
scylla-4.x-215-test-3node
branch
2 times, most recently
from
4c6e0b1
to
6281ff1
Compare
|
I'll squash review adjustments commit into others before merge. |
Bouncheck
force-pushed
the
scylla-4.x-215-test-3node
branch
2 times, most recently
from
5a302d9
to
18ac7ff
Compare
Bouncheck
force-pushed
the
scylla-4.x-215-test-3node
branch
from
18ac7ff
to
a6754ed
Compare
| @@ -16,7 +19,8 @@ public class ResolverProvider { | |||
| * @param clazz Class that is requesting the {@link Resolver}. | |||
| * @return new {@link Resolver}. | |||
| */ | |||
| public static Resolver getResolver(Class<?> clazz) { | |||
| public static synchronized Resolver getResolver(Class<?> clazz) { | |||
There was a problem hiding this comment.
To make it work correctly in 100% cases you better use ReadWriteLock, synchronized does not solve any problem here.
There was a problem hiding this comment.
I think it solves the problem I described. With synchronized you cannot have getResolver race with setDefaultResolverFactory and it's simpler
There was a problem hiding this comment.
Actually with synchronized i think we can have normal booleans instead
There was a problem hiding this comment.
I think it solves the problem I described. With
synchronizedyou cannot havegetResolverrace withsetDefaultResolverFactoryand it's simpler
I am pretty sure it is not the case, can you make a test to test it.
There was a problem hiding this comment.
You are correct, let's convert all of them to regular attributes
Bouncheck
force-pushed
the
scylla-4.x-215-test-3node
branch
2 times, most recently
from
3b40118
to
1d8f6f2
Compare
|
@Bouncheck , can you please address test failures on CICD |
|
Created #340 . It should solve the 6.1.1 failures. |
should_connect_with_mocked_hostname() will use CcmBridge.Builder builder instead.
Bouncheck
force-pushed
the
scylla-4.x-215-test-3node
branch
3 times, most recently
from
55e1557
to
77bca82
Compare
Adds another method that runs scenario in which we replace three node cluster with the completely new three node cluster. This method runs 20 times called by `run_replace_test_20_times()` test method.
Adds "--wait-other-notice", "--wait-for-binary-proto" if missing.
Bouncheck
force-pushed
the
scylla-4.x-215-test-3node
branch
from
77bca82
to
e9d0b9c
Compare
|
@Bouncheck , is it ready ? |
|
Yes, but it was a little flaky with cassandra. Should i reduce number of nodes to 2? |
I see it is fine, let's address it later. |
Adds
MockResolverIT#replace_cluster_test()as a test method that runs the replace cluster scenario and checks if driver managed to reconnect.In the test we create three node cluster and replace it with completely new one. Hostname is mocked and points to all 3 nodes.