vert-x3 / issues

Apache License 2.0
37 stars 7 forks source link

Resolve distributed build dependency #416

Closed vietj closed 6 years ago

vietj commented 6 years ago

Currently the stack build has a cyclic dependency that does not appear when building projects separately:

[ERROR] [ERROR] The projects in the reactor contain a cyclic reference: Edge between 'Vertex{label='io.vertx:vertx-health-check:3.6.0-SNAPSHOT'}' and 'Vertex{label='io.vertx:vertx-service-discovery:3.6.0-SNAPSHOT'}' introduces to cycle in the graph io.vertx:vertx-service-discovery:3.6.0-SNAPSHOT --> io.vertx:vertx-hazelcast:3.6.0-SNAPSHOT --> io.vertx:vertx-health-check:3.6.0-SNAPSHOT --> io.vertx:vertx-service-discovery:3.6.0-SNAPSHOT @ 

with the cycle:

vertx-health-check -> vertx-service-discovery -> vertx-hazelcast -> vertx-health-check

I've resolved temporarily it in the simplest manner I could find : I removed the dependency from vertx-health-check to vertx-service-discovery

This happens mainly because we have some integration level between these modules.

This cycle needs to be resolved properly for 3.6.0

vietj commented 6 years ago

assigned @tsegismont and @cescoffier so you have a look at the issue and suggest a better resolution than the simplest one I could find: https://github.com/vert-x3/vertx-health-check/commit/c3a110116c7095faaedafbd09338318076f838ea

please discuss here.

tsegismont commented 6 years ago

@vietj note the same issue applies with vertx-infinispan

tsegismont commented 6 years ago

How about moving the Infinispan and Hazelcast health checks to the vertx-health-check repo?

vietj commented 6 years ago

that would make sense to me, WDYT @cescoffier ?

cescoffier commented 6 years ago

Yes, we can do this and make the health check project a multi-module project:

health-checks
infinispan-health-check
hazelcast-health-check

WDYT?

vietj commented 6 years ago

makes sense to me.

I can give a try this week before the first CR.

vietj commented 6 years ago

actually now I think about it, I am not inclined to do that because it means that

vietj commented 6 years ago

I think instead we should try to break the link from service-discovery to hazelcast (I don't know the nature of it for now), I believe that most integration should be done in cluster managers when it happens because those are terminal nodes of the stack build graph.

cescoffier commented 6 years ago

Hazlecast is used for testing service discovery when running in clustered mode.

vietj commented 6 years ago

yep I understand that, could we test like we do with vertx-web ?

vietj commented 6 years ago

vertx-web clustered session integration tests have been moved to cluster managers

cescoffier commented 6 years ago

That would be very weird to test the default backend of service discovery in the cluster managers:

  1. it would duplicate the test, while it does not stress any particular hard topic
  2. if we go this way, all tests testing in clustered mode will end up into the cluster managers - so the cluster managers will potentially depend on everything.
vietj commented 6 years ago

I think you should look at how it is done in vertx-web and the cluster managers.

Conversely it's weird to have this tested for Hazelcast and not Infinispan, Ignite and Zookeeper.

cescoffier commented 6 years ago

I picked the one that was considered as the default one.

cescoffier commented 6 years ago

I don't think putting tests in the cluster-managers is a good idea, especially for this case where we just want to check it works for this specific use case.

vietj commented 6 years ago

I think that for checking this feature functionnally, using the fake cluster manager is correct to be used

tsegismont commented 6 years ago

using the fake cluster manager is correct to be used

Agreed. Such tests participate in building a "tck" for cluster manager implementations.

cescoffier commented 6 years ago

Is the fake cluster manager using codecs?

cescoffier commented 6 years ago

Can we artificially increase the latency?

vietj commented 6 years ago

for which operations ?