Skip to content

Conversation

@snvijaya
Copy link
Contributor

@snvijaya snvijaya commented Sep 2, 2019

Enabling config option to control the SSL channel mode. Possible channel modes being -
(a) OpenSSL
(b) Default_JSE
(c) Default - This is the default choice if the config is not present or is invalid. When set to this, connection creation is attempted in OpenSSL mode and will fallback to Default_JSE mode on any failure.

try {
fs = (AdlFileSystem)
(AdlStorageConfiguration.createStorageConnector(conf));
} catch (URISyntaxException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

just add the URISyntaxException to the list of exceptions this test may throw and remove the try/catch

}

SSLChannelMode sslChannelMode = fs.getAdlClient().getSSLChannelMode();
Assert.assertTrue("Channel mode needs to be OpenSSL",
Copy link
Contributor

Choose a reason for hiding this comment

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

assertEquals for the detailed exception message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have re-formatted the test code to reduce redundant code. Please check.

@steveloughran
Copy link
Contributor

  1. Are the options case insensitive? If so, can you document this?
  2. In "Default", does the user get told that they can't load openssl? As the (reverted) s3a one did this and it just added yet another noisy log message everywhere.

@steveloughran
Copy link
Contributor

can you take a look at #970?

assuming that went in, how would your patch link up with it? At the very least, we need consistent option values, defaults, error text, so its easiest to document and use

@snvijaya
Copy link
Contributor Author

snvijaya commented Sep 4, 2019

  1. Are the options case insensitive? If so, can you document this?
    [Sneha]: Have added the case insensitive option to core-default.xml value description.
  2. In "Default", does the user get told that they can't load openssl? As the (reverted) s3a one did this and it just added yet another noisy log message everywhere.
    [Sneha]: A log line is printed only if SSLContext with OpenSSL could not be created.
    https://github.com/Azure/azure-data-lake-store-java/blob/master/src/main/java/com/microsoft/azure/datalake/store/SSLSocketFactoryEx.java [line 167]

@snvijaya
Copy link
Contributor Author

snvijaya commented Sep 4, 2019

can you take a look at #970?

assuming that went in, how would your patch link up with it? At the very least, we need consistent option values, defaults, error text, so its easiest to document and use

[Sneha]: Config inputs are inline to the above S3 commit.

@steveloughran
Copy link
Contributor

changes in hadoop code are good.

In "Default", does the user get told that they can't load openssl? As the (reverted) s3a one did this and it just added yet another noisy log message everywhere.

A log line is printed only if SSLContext with OpenSSL could not be created.

This means that by default a warning is going to be printed unless openssl is happy. I'm not convinced that is actually useful to most people, and I'm going to make sure that when s3a adds open SSL it isn't going to bother logging at info. However: its in your SDK, so your choice.

@steveloughran
Copy link
Contributor

+1, committed to trunk. If you want it in earlier versions just cherrypick, retest and provide a PR which merges. I'm assuming you will want to do this, so I'm leaving it open

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.

2 participants