Skip to content

[SPARK-19409][BUILD][test-maven] Fix ParquetAvroCompatibilitySuite failure due to test dependency on avro #16795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dongjoon-hyun wants to merge 6 commits into from
Closed

[SPARK-19409][BUILD][test-maven] Fix ParquetAvroCompatibilitySuite failure due to test dependency on avro #16795

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2cc8653
[SPARK-19409][BUILD] Fix ParquetAvroCompatibilitySuite failure due to…
dongjoon-hyun Feb 3, 2017
499f6fd
Move to parent pom.
dongjoon-hyun Feb 6, 2017
42ff642
Revert "Move to parent pom."
dongjoon-hyun Feb 6, 2017
941bf00
Add spark-tags into mesos module.
dongjoon-hyun Feb 6, 2017
47e9033
Use 1.8.1 instead of 1.8.0.
dongjoon-hyun Feb 7, 2017
b9e336f
Add comments.
dongjoon-hyun Feb 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions sql/core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@
<artifactId>parquet-avro</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.avro</groupId>
Copy link
Member

@srowen srowen Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry I missed this, yes we should get this in ASAP.

Is this the only module that would need this updated dependency? we could declare this in the parent's dependencyManagement section in test scope.

Does the "1.8.0" need to sync with a Parquet or other Avro versions -- is there or should there be a property tying them together?

Copy link
Member

@robbinspg robbinspg Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this issue only a test dependency? I see avro.version declared in the top level pom.xml

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for review, @srowen and @robbinspg .

  • Yes. This is the only module.
  • 1.8.0 cames from Parquet here.
  • Top-level avro.version is synced with Hadoop avro version. Here, apache.parquet.avro.AvroParquetWriter in the testcode only requires that.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it into parent pom and am testing now.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Feb 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srowen .
Maven rejects the newly added test dependency, so I reverted the commit about moving into parent pom. To use different versions of avro, it seems we should keep this here.

<artifactId>avro</artifactId>
<version>1.8.0</version>
Copy link
Member

@srowen srowen Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions still -- should this not just use the same version of Avro as the rest of the project? avro.version is already defined to be 1.7.7 in the parent POM. I'd think we need to use that instead.

Second, is this really just a test issue? sql/hive depends on avro. It may be that really sql/core needs to depend on avro too -- in non-test scope. In fact, mvn dependency:tree shows that sql/core already depends on it in compile scope transitively.

Therefore, I wonder if it is best to simply remove the version and scope here. That may be the most correct thing?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Feb 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for review again.

should this not just use the same version of Avro as the rest of the project? avro.version is already defined to be 1.7.7 in the parent POM. I'd think we need to use that instead.

  1. Do you mean to upgrade avro from 1.7.7 into 1.8.0 (or 1.8.1) for the whole Spark project at this time?

Second, is this really just a test issue?

  1. While sql/hive depends on avro 1.7.7 API, ParquetAvroCompatibilitySuite uses AvroParquetWriter which depends on 1.8.0+ only.

Therefore, I wonder if it is best to simply remove the version and scope here. That may be the most correct thing?

  1. If we remove the version, the avro means 1.7.7. It will not resolve the maven build failure.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Feb 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1, I'll create another PR since maven build seems to be unstable for me. It fails due to timeout frequently during this PR.

<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down