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.
                <slf4j.version>1.7.7</slf4j.version>
                <guava.version>18.0</guava.version>
                <akka.version>2.3.7</akka.version>
    -           <java.version>1.7</java.version>
    +           <java.minor.version>1.7</java.minor.version>
    --- 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.<clinit>(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.
---

Reply via email to