Skip to content

Issue 935: Issue 915: Part 3 - Make circe-checksum module work for both java 8 and java 9#937

Closed
sijie wants to merge 6 commits intoapache:masterfrom
sijie:fix_pulsar_checksum
Closed

Issue 935: Issue 915: Part 3 - Make circe-checksum module work for both java 8 and java 9#937
sijie wants to merge 6 commits intoapache:masterfrom
sijie:fix_pulsar_checksum

Conversation

@sijie
Copy link
Member

@sijie sijie commented Jan 3, 2018

Descriptions of the changes in this PR:

- switch from jmockit to mockito, and from testng to junit. make testing and mocking framework consistent across modules.
- skip `-Werror` check in circe-checksum since `finalize` is deprecated at java 9
- change CRC32CDigestManager to use `circe-checksum` module
- fix shading to include `circe-checksum`

This change is based #917 . Gitsha 73049f8 contains the changes to make circe-checksum module work for both java8 and java9.

sijie added 4 commits January 2, 2018 17:13
- add circe into notice files
…th java 8 and java 9

- switch from jmockit to mockito, and from testng to junit. make testing and mocking framework consistent across modules.
- skip `-Werror` check in circe-checksum since `finalize` is deprecated at java 9
- change CRC32CDigestManager to use `circe-checksum` module
- fix shading to include `circe-checksum`
@sijie sijie self-assigned this Jan 3, 2018
@sijie sijie requested a review from merlimat January 3, 2018 03:53
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@jvrao
Copy link
Contributor

jvrao commented Jan 4, 2018

@sijie what is the benefit of this change?

@sijie
Copy link
Member Author

sijie commented Jan 4, 2018

@jvrao

is your question for benefits of moving pulsar-checksum to bookkeeper? or benefits of making it run with java8 and java9?

for "benefits of moving pulsar-checksum to bookkeeper": since pulsar depends on bookkeeper, this module is used for both bookkeeper and pulsar, it is making sense for pushing this module down to bookkeeper, so it doesn't have recursive dependency.

for "benefits of making it run with java8 and java9": currently the CI validates the builds at both java9 and java8. I need to make changes to make this checksum module can be compiled and run at both java9 and java8.

Hope this explains your question here.

@sijie
Copy link
Member Author

sijie commented Jan 5, 2018

retest this please

@sijie
Copy link
Member Author

sijie commented Jan 5, 2018

Merging this change is blocked by #952. Because nar/g++ doesn't work when CI workspace path contains spaces.

@sijie
Copy link
Member Author

sijie commented Jan 5, 2018

both java8 and java9 can compile now with separated ci jobs (added by #953).

sijie added a commit that referenced this pull request Jan 7, 2018
Descriptions of the changes in this PR:

*Problem*

When using a matrix job to define a CI job running on both java 9 and java 8, the workspace file path will be added with "jdk name" (e.g. "JDK 1.8 latest" and "JDK 9 latest"). This will cause file paths contain spaces, and fail the builds.

for example, #879 #880 is one of the examples. #937 can't pass ci because g++ fails when the file path contains spaces.

*Solution*

Splitting the matrix job into two separate jobs suffixed with "java8" and "java9", so it won't contains any space in workspace.

Author: Sijie Guo <sijie@apache.org>

Reviewers: Enrico Olivelli <eolivelli@gmail.com>

This closes #953 from sijie/separate_jdk_cis, closes #952
@sijie sijie added this to the 4.7.0 milestone Jan 7, 2018
@sijie sijie closed this in 1e2c24b Jan 7, 2018
@sijie sijie deleted the fix_pulsar_checksum branch July 16, 2018 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants