Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1400 Helper scripts run custom JAR #1445

Merged
merged 7 commits into from
Jul 24, 2020

Conversation

AdrianOlosutean
Copy link
Contributor

Closes #1400

@AdrianOlosutean AdrianOlosutean changed the title #1400 Assign SPARK_JOBS_JAR with default #1400 Helper scripts run custom JAR Jul 16, 2020
@@ -26,7 +26,7 @@ SPARK_SUBMIT="$SPARK_HOME/bin/spark-submit"

HDP_VERSION="2.7.3"

SPARK_JOBS_JAR="enceladus-spark-jobs.jar"
SPARK_JOBS_JAR="${SPARK_JOBS_JAR:="enceladus-spark-jobs.jar"}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, I like the solution and it will work. but then this doesn't seem like a plain environment script.

How about introducing SPARK_JOBS_JAR_OVERRIDE or something that is then used in run_standardization.sh like export JAR=${SPARK_JOBS_JAR_OVERRIDE:=$SPARK_JOBS_JAR}

Basically using your solution but in a lower place.

Copy link
Contributor Author

@AdrianOlosutean AdrianOlosutean Jul 16, 2020

Choose a reason for hiding this comment

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

So then it would be modified in more places? How about something like export SPARK_JOBS_JAR=${SPARK_JOBS_JAR:=$SPARK_JOBS_JAR_OVERRIDE} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want evaluation and user overriding in ENV script. Yes, it would be in more places. 2 places vs 1. run_standardization.sh and run_conformance.sh. Or you can try and do it in run_enceladus.sh.

Now that I am looking into run_enceladus.sh I see

    --jar)
    JAR="$2"
    shift 2 # past argument and value
    ;;

What does it do? Does it do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so I think it's about modifying run_standardization.sh and run_conformance.sh to include this override.

Copy link
Contributor

@Zejnilovic Zejnilovic left a comment

Choose a reason for hiding this comment

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

Did you already find out what the --jar does?

@@ -18,7 +18,7 @@ SRC_DIR=`dirname "$0"`
source ${SRC_DIR}/enceladus_env.sh

export CLASS=${CONF_CLASS}
export JAR=${SPARK_JOBS_JAR}
export JAR=${SPARK_JOBS_JAR:=$SPARK_JOBS_JAR_OVERRIDE}
Copy link
Contributor

Choose a reason for hiding this comment

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

for it to be an override you would have to do

Suggested change
export JAR=${SPARK_JOBS_JAR:=$SPARK_JOBS_JAR_OVERRIDE}
export JAR=${SPARK_JOBS_JAR_OVERRIDE:=$SPARK_JOBS_JAR}

Copy link
Contributor

@yruslan yruslan Jul 17, 2020

Choose a reason for hiding this comment

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

--jar allows you to override the jar location from the command line. There is also --class, but there is a bug in the script and it doest it wrong. You can fix --class if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about the user having to specify SPARK_JOBS_JAR_OVERRIDE so it is a bit more explicit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Left a comment: #1400 (comment)

Zejnilovic
Zejnilovic previously approved these changes Jul 17, 2020
Copy link
Contributor

@Zejnilovic Zejnilovic left a comment

Choose a reason for hiding this comment

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

Clicked the button update branch so I can approve

@@ -18,7 +18,7 @@ SRC_DIR=`dirname "$0"`
source ${SRC_DIR}/enceladus_env.sh

export CLASS=${CONF_CLASS}
export JAR=${SPARK_JOBS_JAR}
export JAR=${SPARK_JOBS_JAR_OVERRIDE:-$SPARK_JOBS_JAR}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move this to run_enceladus.sh so it's actually present in the code only once.

The separation is due to historical reasons, when Standardization and Conformance were in different jar files.

Copy link
Contributor

@Zejnilovic Zejnilovic left a comment

Choose a reason for hiding this comment

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

This ended up being an easy task ... 😄

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@AdrianOlosutean AdrianOlosutean merged commit 109ccbf into develop Jul 24, 2020
@AdrianOlosutean AdrianOlosutean deleted the feature/1400-helper-scripts-run-custom-jar branch July 24, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve helper scripts to run custom JARs
5 participants