Feature/350 fix tests#351
Conversation
JaCoCo code coverage report
|
| <!-- Avro --> | ||
| <dependency> | ||
| <groupId>org.apache.avro</groupId> | ||
| <artifactId>avro</artifactId> | ||
| <version>${avro.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
|
|
There was a problem hiding this comment.
Since the dependency is not declared, where is the Avro coming from? Confluent or Spark?
There was a problem hiding this comment.
It's coming from Spark. But I realize that it's basically luck, I didn't think of confluent.
The reason why I removed it is because in principle we should compile against the Avro version that Spark sets, because Avro is packaged with the Spark runtime.
We could just set the Avro version for each Spark version, but we also need to set the version for jackson-core, because Spark does actually override it. E.g. Avro 1.11.2 depends on jackson-core 2.14.2, but Spark 3.5.0 overrides this and uses jackson-core 2.15.2
Another, perhaps radical approach would be to import the pom of spark-parent, and thereby its dependencyManagement section (https://github.com/apache/spark/blob/master/pom.xml#L434-L2857). Then we would be sure that we always use the same dependencies as Spark does. However, unfortunately it's overly intrusive since it defines not only the runtime dependencies, but also test dependencies like scalatest, scalacheck etc.
So for now, I think the better solution is to just define the versions of avro and jackson-core along with the spark version in the respective profiles
There was a problem hiding this comment.
Another, perhaps radical approach would be to import the pom of spark-parent
I did this for spline agent, but we have separated pom for the uber-jar creation there if I remember correctly.
But I agree, this is a good solution.
| <spark.version>${spark-35.version}</spark.version> | ||
| <confluent.version>6.2.1</confluent.version> | ||
| </properties> | ||
| </profile> |
There was a problem hiding this comment.
confluent.version seems to be the same everywhere.
There was a problem hiding this comment.
Yes, I guess we can set it as a constant then. If we want to make it dynamic it in the future, we can always change it
cerveada
left a comment
There was a problem hiding this comment.
LGTM, just read the code
Closes #350