DBZ-8363 creation of single test artifact #136
Conversation
There was a problem hiding this comment.
@smiklosovic Thanks a lot! I really like the approach. The only comments I do have relates to changes in CassandraConnectorConfig. If something new is intorduced there then there should be a separate Jira (enhancement) and separate commit for it. The new config options would need a documentation PR too.
core/src/main/java/io/debezium/connector/cassandra/CassandraConnectorConfig.java
Outdated
Show resolved
Hide resolved
| public static final Field CASSANDRA_CLUSTER_NAME = Field.create("cassandra.cluster.name") | ||
| .withType(Type.STRING) | ||
| .withDescription("Name of Cassandra cluster.") | ||
| .withDefault("Test Cluster"); |
There was a problem hiding this comment.
Why it was not necessary to override it before?
There was a problem hiding this comment.
It is used in production. In CassandraConnectorTaskTemplate in initProcessorGroup. Before that, there was deserializerProvider.getClusterName() at taskContext.getClusterName() place.
processorGroup.addProcessor(new SnapshotProcessor(taskContext, taskContext.getClusterName()));
Now if you think, about that, what the hell does CassandraTypeProvider have in common with name of the cluster? It was implemented like this:
@Override
public String getClusterName() {
return DatabaseDescriptor.getClusterName();
}
So, the only reason we put this into CassandraTypeProvider before was that we were trying to "hide" this somewhere so DatabaseDescriptor is on the class path and it is module specific. We could not put this into "core" because DatabaseDescriptor is not there (we are not depending on any specific Cassandra version there).
Cluster name should be fetched from CassandraConnectorContext. We have one implementation of the context - DefaultCassandraConnectorContext which is in core, in CassandraConnectorTaskTemplate. So we can not put there DatabaseDescriptor.getClusterName because that class is not in core. Hence I made a configuration property out of that.
The same logic holds for the other property but I see it is used in tests only so I will remove the other one but cluster name has to stay.
There was a problem hiding this comment.
I am not sure how to deliver this PR while we would not refactor the configuration option at the same time like here.
I would have to move changes in CassandraTypeProvider back (even they are pretty nonsensical) just for the sake of delivering this patch and then we would need to create yet another JIRA to fix it.
Is this what you indeed prefer me to do?
There was a problem hiding this comment.
@smiklosovic We've also createInternal to create field definition. So it might be fine to use it and just add proper description.
There was a problem hiding this comment.
@jpechane I created https://issues.redhat.com/browse/DBZ-8373
I have removed it and put it back to CassandraTypeProvider even it does not make sense.
There was a problem hiding this comment.
@jpechane ah, I noticed you comment just now ... I am not completely sure if createInternal is the right thing to do. It is not internal. I think that property (cluster name) which is used in SnapshotProcessor uses name of cluster internally etc so I think that property is really something a user should have a control over.
There was a problem hiding this comment.
So, lte me rephrase it. If it is something that user can and should use then let's keep it as is. I don't like the default value but it is apparently matching the one in driver so I am going to accept it.
BTW, there slight misunderstanding. I did not ask originally for the removal, just explanation.
There was a problem hiding this comment.
@jpechane I am fine to rework it in DBZ-8373 just to be more explicit about that. If it was delivered with this patch then it would be all burried in this big patch with all changes. Let's just deliver this as it is now and then we can refactor in the other one. That is totally fine to me.
core/src/main/java/io/debezium/connector/cassandra/CassandraConnectorConfig.java
Outdated
Show resolved
Hide resolved
|
@jpechane thank you for the feedback, I shall get back to this next week. |
smiklosovic
force-pushed
the
DBZ-8363
branch
2 times, most recently
from
e9f9d9e
to
4447f1e
Compare
|
not sure why the PR stopped to be built automatically |
|
@smiklosovic I see it built |
|
@smiklosovic Deal done, applied. Thanks! |
@jpechane
This PR introduces a single test artifact which is made a dependency of each respective implementation module. By doing that, we run all tests (when not excluded programmatically) for each module while tests are written just once.
The solution uses ServiceLoader mechanism for loading bits which differ and tests load this and use it.
For review purposes, it is adviced if reviewers open the code in IDE to see what is going on here.
From development perspective, the workflow is that when a new test is being written, it would be written in respective module but upon commit, it would need to be moved into tests artifact. Upon development, a developer will very likely want to have a quick feedback loop and placing test into tests artifact means he is not able to test that individual test in IDE. Hence, a test would be developed in some module but it would be moved to tests upon commit. Another option is to run just one test via Maven "somehow" but a developer very likely wants to debug in IDE as well, hence the first approach is suggested.
If that test class is meant to be run just for a specific Cassandra version, it might stay in the implementation module instead of the tests module.