[8n02RETv] Fixes the cluster setup for the trigger tests #322
Conversation
ncordon
force-pushed
the
fixes-cluster-setup
branch
from
de9f587
to
ac69fde
Compare
common/build.gradle
Outdated
| @@ -56,7 +56,7 @@ dependencies { | |||
| api group: 'org.hdrhistogram', name: 'HdrHistogram', version: '2.1.9' | |||
| api group: 'org.apache.commons', name: 'commons-collections4', version: '4.2' | |||
| // We need this to avoid seeing SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder" on startup | |||
| api group: 'org.slf4j', name: 'slf4j-simple', version: '1.7.36' | |||
| api group: 'org.slf4j', name: 'slf4j-simple', version: '2.0.0' | |||
There was a problem hiding this comment.
Without this change the cluster containers complain the logger is too old and they don't log anything
| @@ -31,7 +31,7 @@ public class TriggerClusterRoutingTest { | |||
| public static void setupCluster() { | |||
| cluster = TestContainerUtil | |||
| .createEnterpriseCluster( List.of(TestContainerUtil.ApocPackage.CORE), | |||
| 3, 1, | |||
| 3, 0, | |||
There was a problem hiding this comment.
Don't allocate a read replica. We weren't testing anything with the read replica anyway (because we were skipping it inside the tests) and read replicas have a different behaviour in 5.x anyway, so it was failing to start up
| final String addTrigger = "CALL apoc.trigger.add($name, 'RETURN 1', {})"; | ||
| final String removeTrigger = "CALL apoc.trigger.remove($name)"; | ||
| String triggerName = randomTriggerName(); | ||
|
|
||
| succeedsInLeader(addTrigger, triggerName, DEFAULT_DATABASE_NAME); | ||
| succeedsInLeader(removeTrigger, triggerName, DEFAULT_DATABASE_NAME); | ||
| failsInFollowers(removeTrigger, triggerName, SYS_DB_NON_WRITER_ERROR, DEFAULT_DATABASE_NAME); |
There was a problem hiding this comment.
I think this reads much nicer than the previous tests, also checks that the remove does work in a LEADER, for what we need to actually add the trigger first.
There was a problem hiding this comment.
I guess it doesn't matter that it tries to remove an already removed trigger in the followers?
There was a problem hiding this comment.
Nope, it would fail before actually doing that
ncordon
changed the title
ncordon
force-pushed
the
fixes-cluster-setup
branch
from
ac69fde
to
3ee6960
Compare
ncordon
force-pushed
the
fixes-cluster-setup
branch
3 times, most recently
from
6c6dcd2
to
4011169
Compare
| final String addTrigger = "CALL apoc.trigger.add($name, 'RETURN 1', {})"; | ||
| final String removeTrigger = "CALL apoc.trigger.remove($name)"; | ||
| String triggerName = randomTriggerName(); | ||
|
|
||
| succeedsInLeader(addTrigger, triggerName, DEFAULT_DATABASE_NAME); | ||
| succeedsInLeader(removeTrigger, triggerName, DEFAULT_DATABASE_NAME); | ||
| failsInFollowers(removeTrigger, triggerName, SYS_DB_NON_WRITER_ERROR, DEFAULT_DATABASE_NAME); |
There was a problem hiding this comment.
I guess it doesn't matter that it tries to remove an already removed trigger in the followers?
What
Solves some cluster tests flakiness.
The setup we had was to wait for every instance to return 200 in the
db/neo4j/cluster/availableendpoint. But in 5.x neo4j is not allocated by default in every instance. This PR forces it to be allocated in every instance and reshapes the trigger tests so they test for what they are supposed to. This is a temporary hack, we should revisit the setup, because we never reshaped it from 4.x (causal clusters) to 5.x (autonomoous cluster).Why
The cluster tests were broken in general and I had realized because there was some flakiness in the CI related to these tests. An instance was always succeeding to start, the one with neo4j allocated in it and if that instance happened to have a be a system leader as well, it was failing for the drop trigger (because we cannot drop something we have not created and drop would return empty results rather than something).