Skip to content

[SPARK-41719][CORE] Skip SSLOptions sub-settings if ssl is disabled#39221

Closed
shrprasa wants to merge 2 commits intoapache:masterfrom
shrprasa:ssl_options_fix
Closed

[SPARK-41719][CORE] Skip SSLOptions sub-settings if ssl is disabled#39221
shrprasa wants to merge 2 commits intoapache:masterfrom
shrprasa:ssl_options_fix

Conversation

@shrprasa
Copy link
Contributor

@shrprasa shrprasa commented Dec 26, 2022

What changes were proposed in this pull request?

In SSLOptions rest of the settings should be set only when ssl is enabled.

Why are the changes needed?

If spark.ssl.enabled is false, there is no use of setting rest of spark.ssl.* settings in SSLOptions as this requires unnecessary operations to be performed to set these properties.
Additional implication of trying to set the rest of settings is if any error occurs in setting these properties it will cause job failure which otherwise should have worked since ssl is disabled. For example, if the user doesn't have access to the keystore path which is set in hadoop.security.credential.provider.path of hive-site.xml, it can result in failure while launching spark shell since SSLOptions won't be initialized due to error in accessing the keystore.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added new test.

@github-actions github-actions bot added the CORE label Dec 26, 2022
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@shrprasa
Copy link
Contributor Author

shrprasa commented Jan 2, 2023

@dongjoon-hyun @holdenk @squito, @HyukjinKwon @yaooqinn Can you please review this PR?

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

In this case, Apache Spark coding style allows the exceptional return statement.

You can do the following simply.

if (!enabled) {
  return new SSLOptions(false)
}

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for making a PR, @shrprasa . I finished the first round review. Could you address the comments?

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-41719] [CORE]: SSLOptions sub settings should be set only when ssl is enabled [SPARK-41719][CORE] Skip SSLOptions sub-settings if ssl is disabled Jan 3, 2023
@shrprasa
Copy link
Contributor Author

shrprasa commented Jan 3, 2023

@dongjoon-hyun Thank you for reviewing the PR. I have made the suggested changes. Can you please review.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you for updating and explanation, @shrprasa .
Merged to master for Apache Spark 3.4.0.

@dongjoon-hyun
Copy link
Member

I added you to the Apache Spark contributor group and assigned SPARK-41719 to you.
Welcome to the Apache Spark community and thank you for your contribution, @shrprasa .

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 4, 2023

BTW, here is a tip for new comers. If you add your commit email, shrprasa@visa.com, to GitHub account as a secondary email additionally, GitHub will show your profile image in the commit log. Currently, your commit doesn't show your image.

https://github.com/apache/spark/commits/master

Screenshot 2023-01-03 at 5 44 38 PM

@shrprasa
Copy link
Contributor Author

shrprasa commented Jan 4, 2023

@dongjoon-hyun Thank you for approving and merging the PR. I have added commit email in github account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments