[GitHub] flink issue #2804: [FLINK-5067] Make Flink compile with 1.8 Java compiler
Github user melentye commented on the issue: https://github.com/apache/flink/pull/2804 @StephanEwen it seems that the semantics is different when java.version is just defined in pom.xml versus being overriden with -Djava.version=x.y.z argument passed to mvn. org.apache.hadoop.hbase.util.ClassSize expects a very specific format of java.version system property. But when java.version is only defined in pom.xml then it doesn't actually become a system property unlike in case of using -Djava.version --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2804: [FLINK-5067] Make Flink compile with 1.8 Java comp...
Github user melentye commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88332568 --- Diff: pom.xml --- @@ -96,7 +96,7 @@ under the License. 1.7.7 18.0 2.3.7 - 1.7 + 1.7 --- End diff -- I actually prefer java.version for it's conciseness and I share the conservative tune in regards to unnecessary pom.xml changes. But in this case I did it on purpose, long explanation follows. The java.version is passed by -Djava.version=1.8 argument to mvn which seems to be Maven-recommended (or even the only one supported) way of redefining the property from command-line. An inconvenient side-effect is that this property is then propagated down until the JVM that executes the tests and causes java.lang.RuntimeException: Unexpected version format: 1.8 at org.apache.hadoop.hbase.util.ClassSize.(ClassSize.java:119) starting in org.apache.flink.addons.hbase.TableInputFormatITCase (won't bore you with the complete stack trace). Since this is third party code, we can't really fix it in Flink source. Therefore I thought to rename the property instead to avoid the clash. There's a way to keep the java.version property name as is: we can use a maven profile to set java.version. In this case when you activate the profile with -P argument to mvn, the java.version will not be propagated as a system property to the JVM running the tests and won't cause problems. I've noticed that there's an existing profile called "jdk8" which sounds relevant. But it's currently automatically activated if you're running Maven with Java 8 so that could turn out to be a surprise for the maintainers to realize that they suddenly are building 1.8 target bytecode. The profile doesn't do much though so I am questioning if it really needs to be auto-activated. I think it'll be cleaner to stop auto-activating it and reuse it for the above purpose. See proposal in the PR update. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #2804: [FLINK-5067] Make Flink compile with 1.8 Java compiler
Github user melentye commented on the issue: https://github.com/apache/flink/pull/2804 @StephanEwen I initially introduced a new Travis profile so that the coverage would be strictly improved, i.e. all old combinations tests plus the new ones. But I am also fine reusing some of the existing profiles too, PR updated. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2804: [FLINK-5067] Make Flink compile with 1.8 Java comp...
Github user melentye commented on a diff in the pull request: https://github.com/apache/flink/pull/2804#discussion_r88331708 --- Diff: flink-tests/src/test/java/org/apache/flink/test/windowing/sessionwindows/SessionWindowITCase.java --- @@ -122,11 +122,11 @@ private void runTest( // check that overall event counts match with our expectations. remember that late events within lateness will // each trigger a window! - Assert.assertEquals( - (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION, + Assert.assertEquals(Long.valueOf( + (LATE_EVENTS_PER_SESSION + 1) * NUMBER_OF_SESSIONS * EVENTS_PER_SESSION), result.getAccumulatorResult(SESSION_COUNTER_ON_TIME_KEY)); - Assert.assertEquals( - NUMBER_OF_SESSIONS * (LATE_EVENTS_PER_SESSION * (LATE_EVENTS_PER_SESSION + 1) / 2), + Assert.assertEquals(Long.valueOf( --- End diff -- Sure thing, fixed --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2804: [FLINK-5067] Make Flink compile with 1.8 Java comp...
Github user melentye commented on a diff in the pull request:
https://github.com/apache/flink/pull/2804#discussion_r88331670
--- Diff:
flink-java/src/test/java/org/apache/flink/api/common/operators/CollectionExecutionAccumulatorsTest.java
---
@@ -35,7 +35,7 @@
@Test
public void testAccumulator() {
try {
- final int NUM_ELEMENTS = 100;
+ final Integer NUM_ELEMENTS = 100;
--- End diff --
Absolutely!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2804: [FLINK-5067] Make Flink compile with 1.8 Java comp...
GitHub user melentye opened a pull request: https://github.com/apache/flink/pull/2804 [FLINK-5067] Make Flink compile with 1.8 Java compiler - New type inference rules changed which overloaded methods are picked. - Renamed java.version property in maven to java.minor.version. This is due to the fact that overriding java.version is used by some third-party classes and they expect the version to be fully qualified. - Introduced Java 8 build for Travis CI You can merge this pull request into a Git repository by running: $ git pull https://github.com/melentye/flink FLINK-5067-make-flink-compile-with-java8-compiler Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2804.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2804 commit f07215eed4269826e57136fa30560a34b8409d34 Author: Andrey Melentyev Date: 2016-11-13T16:42:17Z [FLINK-5067] Java 8 build fixes - New type inference rules changed which overloaded methods are picked. - Renamed java.version property in maven to java.minor.version. This is due to the fact that overriding java.version is used by some third-party classes and they expect the version to be fully qualified. - Introduced Java 8 build for Travis CI --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2760: [FLINK-5014] [RocksDB backend] add toString for Ro...
GitHub user melentye opened a pull request:
https://github.com/apache/flink/pull/2760
[FLINK-5014] [RocksDB backend] add toString for RocksDBStateBackend
Adding toString() for RocksDBStateBackend.
Example output 1:
> RocksDB State Backend {isInitialized=false, configuredDbBasePaths=null,
initializedDbBasePaths=null, checkpointStreamBackend=File State Backend @
file:/Users/melentye/tmp/flink-checkpoints}
Example output 2:
> RocksDB State Backend {isInitialized=true,
configuredDbBasePaths=[/var/folders/k_/t4spznpj543cgt7vt59_6jp0gn/T/junit6807913496462961318/junit8522690214676090832],
initializedDbBasePaths=[/var/folders/k_/t4spznpj543cgt7vt59_6jp0gn/T/junit6807913496462961318/junit8522690214676090832],
checkpointStreamBackend=File State Backend @
file:/var/folders/k_/t4spznpj543cgt7vt59_6jp0gn/T/junit6807913496462961318/junit3625485689993452525}
Normally I wouldn't cover toString method with unit tests, let me know if
that's something we'd like to have in this particular case.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/melentye/flink
FLINK-5014-add-toString-to-RocksDBStateBackend
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2760.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2760
commit 131ce7213c4b7581a6b81ebd87683aa1375ef2df
Author: Andrey Melentyev
Date: 2016-11-05T07:18:38Z
[FLINK-5014] [RocksDB backend] add toString for RocksDBStateBackend
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
