Add support for configurable JDKs #316

sfcoy · 2020-10-21T06:43:10Z

This PR makes the following changes:

Adds support for user configurable JDKs in order to enable JDK 11 or newer to be installed

I tested this PR by...

Creating and running Java 8 based clusters
Creating and running Java 11 based clusters

nchammas

Looks very good to me! Left some minor comments about the details.

One other thought that comes to me is that it would be useful to add Java 8 vs. 11 to the acceptance tests somehow. Not that every test should run twice, once with Java 8 and once with Java 11, but that there should be at least one test that somehow makes sure this Java flag is working for some common use case.

Do you think there is a simple way we can add a test like that?

flintrock/core.py

flintrock/flintrock.py

…dk_repo function Change jdk option name to java-version/java_version and make it an int If the correct version of Java is already installed then do not install again Add warning if the java version is being downgraded

* add spot request duration (looks like this was not working before?) * add java version

nchammas

@sfcoy, this looks great! I think a couple final tweaks and it's ready to go. I will also make time to test it myself this week.

By the way, a request: If you can, please avoid force pushing the branch. It disconnects review comments from the PR commit history and makes it a bit harder to follow review comments over time.

I'm guessing you did it to overcome some troubles updating the branch from the latest master? At this point it may be easier to just manually delete the spot request duration stuff and push a new commit. I will squash the whole PR into a single commit when merging to master, anyway.

flintrock/core.py

flintrock/flintrock.py

Added more documentation

sfcoy

One other thought that comes to me is that it would be useful to add Java 8 vs. 11 to the acceptance tests somehow. Not that every test should run twice, once with Java 8 and once with Java 11, but that there should be at least one test that somehow makes sure this Java flag is working for some common use case.

Do you think there is a simple way we can add a test like that?

It would be nice if we could. However I am not currently up with modern python testing methodology (I'm a Java guy).
Right now I manually test four scenarios:

Vanilla AMI with no Java installed
Java 11 AMI with java-version: 8
Java 11 AMI with java-version: 11
Java 11 AMI with java-version: 14

I had to build my own Java 11 AMI because the only existing ones seem to require subscriptions and AWS was being cranky about these today (returning 400 errors).

flintrock/flintrock.py

sfcoy · 2020-11-02T01:00:26Z

flintrock/flintrock.py

@@ -722,6 +726,8 @@ def stop(cli_context, cluster_name, ec2_region, ec2_vpc_id, assume_yes):
              help="Path to SSH .pem file for accessing nodes.")
 @click.option('--ec2-user')
 @click.option('--ec2-spot-price', type=float)
+@click.option('--ec2-spot-request-duration', default='7d',


Hi @nchammas ,
I suspect that "add-slaves" in master is not working right now because this "ec2_spot_request_duration" parameter is missing.

Good catch. I'll report this over on #315.

flintrock/core.py

flintrock/flintrock.py

nchammas · 2020-12-27T04:11:00Z

Just noting for the record that this issue potentially closes #313.

@sfcoy - Sorry about the delay in reviewing the latest changes here. I intend to get to them this upcoming week and get this merged in!

nchammas

This PR is good to go. If I can trouble you for one more improvement, it would be to display a warning if the user launches a cluster with Hadoop < 3.3 and Java 11. Only Hadoop 3.3+ are compatible with Java 11+.

If you try to launch a cluster with Hadoop 3.2 and Java 11, you get an error like this on starting up HDFS:

2021-01-04 00:17:27,413 ERROR [main] namenode.NameNode (NameNode.java:main(1764)) -
 Failed to start namenode.
java.lang.ExceptionInInitializerError
        at org.eclipse.jetty.webapp.WebInfConfiguration.findAndFilterContainerPaths
(WebInfConfiguration.java:185)
...
Caused by: java.lang.IllegalArgumentException: Invalid Java version 11.0.9.1

I would probably add the check somewhere here:

flintrock/flintrock/flintrock.py

Lines 406 to 412 in 22623d4

    
           if install_hdfs: 
        
               validate_download_source(hdfs_download_source) 
        
               hdfs = HDFS( 
        
                   version=hdfs_version, 
        
                   download_source=hdfs_download_source, 
        
               ) 
        
               services += [hdfs]

No worries if you can't get to it. In that case, I will push an appropriate fix to this branch before merging it in, if you don't mind.

flintrock/core.py

nchammas · 2021-01-03T23:03:29Z

flintrock/flintrock.py

@@ -722,6 +726,8 @@ def stop(cli_context, cluster_name, ec2_region, ec2_vpc_id, assume_yes):
              help="Path to SSH .pem file for accessing nodes.")
 @click.option('--ec2-user')
 @click.option('--ec2-spot-price', type=float)
+@click.option('--ec2-spot-request-duration', default='7d',


Good catch. I'll report this over on #315.

sfcoy · 2021-01-05T00:39:39Z

This PR is good to go. If I can trouble you for one more improvement, it would be to display a warning if the user launches a cluster with Hadoop < 3.3 and Java 11. Only Hadoop 3.3+ are compatible with Java 11+.

If you try to launch a cluster with Hadoop 3.2 and Java 11, you get an error like this on starting up HDFS:
2021-01-04 00:17:27,413 ERROR [main] namenode.NameNode (NameNode.java:main(1764)) -
 Failed to start namenode.
java.lang.ExceptionInInitializerError
        at org.eclipse.jetty.webapp.WebInfConfiguration.findAndFilterContainerPaths
(WebInfConfiguration.java:185)
...
Caused by: java.lang.IllegalArgumentException: Invalid Java version 11.0.9.1
I would probably add the check somewhere here:

flintrock/flintrock/flintrock.py

Lines 406 to 412 in 22623d4

if install_hdfs:

validate_download_source(hdfs_download_source)

hdfs = HDFS(

version=hdfs_version,

download_source=hdfs_download_source,

)

services += [hdfs]

No worries if you can't get to it. In that case, I will push an appropriate fix to this branch before merging it in, if you don't mind.

The Hadoop team must have added a Java version check, because I have actually built and run this combination before.

nchammas · 2021-01-08T01:38:08Z

I've filed #322 to track adding some sort of compatibility check to Flintrock.

Merging this in. Thank you for persisting through the review process @sfcoy and getting this important enhancement in!

sfcoy · 2021-01-10T03:55:23Z

I've filed #322 to track adding some sort of compatibility check to Flintrock.

Merging this in. Thank you for persisting through the review process @sfcoy and getting this important enhancement in!

No worries mate. I was actually becoming a little concerned about you, given the Covid situation and all.

* Add support for configurable JDKs * Perform complete adoptopenjdk.repo installation in install_adoptopenjdk_repo function Change jdk option name to java-version/java_version and make it an int If the correct version of Java is already installed then do not install again Add warning if the java version is being downgraded * Fix add_slaves functionality * add spot request duration (looks like this was not working before?) * add java version * Cleaned up and clarified the Java installation logic. Added more documentation * tweak wording of docstring Co-authored-by: Nicholas Chammas <[email protected]>

nchammas reviewed Oct 25, 2020

View reviewed changes

sfcoy added 2 commits October 30, 2020 15:38

Add support for configurable JDKs

897c4ba

sfcoy force-pushed the multi-jdk-support branch from 5d53804 to 45e1523 Compare October 30, 2020 07:25

Fix add_slaves functionality

22623d4

* add spot request duration (looks like this was not working before?) * add java version

sfcoy force-pushed the multi-jdk-support branch from 5c455a9 to 22623d4 Compare November 2, 2020 07:11

nchammas reviewed Nov 9, 2020

View reviewed changes

flintrock/core.py Outdated Show resolved Hide resolved

flintrock/core.py Outdated Show resolved Hide resolved

flintrock/flintrock.py Show resolved Hide resolved

Cleaned up and clarified the Java installation logic.

9dea764

Added more documentation

sfcoy commented Nov 22, 2020

View reviewed changes

nchammas added the needs changelog label Jan 3, 2021

nchammas mentioned this pull request Jan 4, 2021

Make Python 3 the default on launched clusters #320

Closed

nchammas approved these changes Jan 4, 2021

View reviewed changes

tweak wording of docstring

a83f79e

nchammas merged commit 6792626 into nchammas:master Jan 8, 2021

nchammas mentioned this pull request Jan 8, 2021

Spark 3.0.0 incompatibilities #313

Closed

nchammas mentioned this pull request Feb 13, 2021

Track Java version in cluster manifest #333

Merged

nchammas removed the needs changelog label Jun 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for configurable JDKs #316

Add support for configurable JDKs #316

sfcoy commented Oct 21, 2020

nchammas left a comment

nchammas left a comment •

edited

Loading

sfcoy left a comment

sfcoy Nov 2, 2020

nchammas Jan 3, 2021

nchammas commented Dec 27, 2020

nchammas left a comment

nchammas Jan 3, 2021

sfcoy commented Jan 5, 2021

nchammas commented Jan 8, 2021

sfcoy commented Jan 10, 2021

	if install_hdfs:
	validate_download_source(hdfs_download_source)
	hdfs = HDFS(
	version=hdfs_version,
	download_source=hdfs_download_source,
	)
	services += [hdfs]

Add support for configurable JDKs #316

Add support for configurable JDKs #316

Conversation

sfcoy commented Oct 21, 2020

nchammas left a comment

Choose a reason for hiding this comment

nchammas left a comment • edited Loading

Choose a reason for hiding this comment

sfcoy left a comment

Choose a reason for hiding this comment

sfcoy Nov 2, 2020

Choose a reason for hiding this comment

nchammas Jan 3, 2021

Choose a reason for hiding this comment

nchammas commented Dec 27, 2020

nchammas left a comment

Choose a reason for hiding this comment

nchammas Jan 3, 2021

Choose a reason for hiding this comment

sfcoy commented Jan 5, 2021

nchammas commented Jan 8, 2021

sfcoy commented Jan 10, 2021

nchammas left a comment •

edited

Loading