Allow compilation as Java8 source and target#3328
Conversation
|
FYI https://github.com/gianm/druid/tree/je is the patch I've used for upgrading Jetty (see https://groups.google.com/forum/#!msg/druid-development/rQMk8C4cUfs/atGoF8XEBQAJ for motivation). Sorta related since the new Jetty requires Java 8. |
drcrallen
added
Difficulty - Medium
Incompatible
Release Notes
Discuss
and removed
Discuss
labels
|
Required because of #3889 (comment) |
…affeine-cache configuration
…Test.testSanity()
|
@gianm @drcrallen this PR passes CI now. |
|
haha, I can't approve my own pull request, thanks github |
|
👍 |
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <configuration> | ||
| <source>1.7</source> |
There was a problem hiding this comment.
Is there a need to still target 1.7 for druid-api?
There was a problem hiding this comment.
Discussed offline with @drcrallen and this is apparently due to an abundance of caution about working with existing extensions. Although, I think this abundance of caution is not abundant enough, since we have extension points like AggregatorFactory that are not in druid-api (see http://druid.io/docs/latest/development/modules.html).
@drcrallen @leventov do you know if it would be enough to do the following:
- Compile everything (including druid-api) for java 1.8 starting in 0.10.1
- Add to the release notes for 0.10.0 "extensions must target java 1.8 to be compatible with future 0.10.x versions"
- Expect that users that follow the above guideline will be able to build an extension for 0.10.0 and have it work with 0.10.1.
If so then this change LGTM
There was a problem hiding this comment.
That sounds good to me. meaning we should change this PR to remove the druid-api special java version.
leventov
changed the title
|
Nevermind, this is all testing code, all good |
|
Actually I believe the rule is that outside of docs, we need 2 +1s, so someone else will need to approve this |
|
@fjy this PR has commits from @drcrallen and me, and we approved each other's commits. The other +1 is from @gianm. |
|
@leventov my +1 was conditional on the question I had above, so I'm guessing the answer is "yes that should be fine"? Pursuant to that I added this to the release notes for 0.10.0:
|
|
Also: opinions on whether we should include this in 0.10.0 too? If we're going to build with jdk8 in 0.10.1 it seems "cleaner" to do it starting from 0.10.0. |
Yes, @drcrallen agreed with that, I also agree. |
* Allow compilation as Java8 source and target for everything except API * Remove conditions in tests which assume that we may run with Java 7 * Update easymock to 3.4 * Make Animal Sniffer to check Java 1.8 usage; remove redundant druid-caffeine-cache configuration * Use try-with-resources in LargeColumnSupportedComplexColumnSerializerTest.testSanity() * Remove java7 special for druid-api
* Allow compilation as Java8 source and target for everything except API * Remove conditions in tests which assume that we may run with Java 7 * Update easymock to 3.4 * Make Animal Sniffer to check Java 1.8 usage; remove redundant druid-caffeine-cache configuration * Use try-with-resources in LargeColumnSupportedComplexColumnSerializerTest.testSanity() * Remove java7 special for druid-api
5 participants
This pull request changes the java source version to java 8.