-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-2950] Support Ceph as a notebook storage #2598
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
Conversation
0fadb86 to
b1ec41a
Compare
| } else { | ||
| this.s3client = new AmazonS3EncryptionClient(credentialsProvider, emp); | ||
| } | ||
| this.s3client = new AmazonS3EncryptionClient(credentialsProvider, emp, cliConf, cryptoConf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The if branch is removed for the simplicity but the internal process of each branch is not changed because new CryptoConfiguration() is called in the new AmazonS3EncryptionClient(credentialsProvider, emp) constructor eventually.
| // use a custom encryption materials provider class | ||
| EncryptionMaterialsProvider emp = createCustomProvider(conf); | ||
| this.s3client = new AmazonS3EncryptionClient(credentialsProvider, emp); | ||
| this.s3client = new AmazonS3EncryptionClient(credentialsProvider, emp, cliConf, cryptoConf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cryptoConf is passed to the new AmazonS3EncryptionClient constructor besides cliConf because there's no constructor definition of new AmazonS3EncryptionClient(credentialsProvider, emp, cliConf).
However, its internal process is not changed because new CryptoConfiguration() is called in the new AmazonS3EncryptionClient(credentialsProvider, emp) constructor eventually.
| } | ||
|
|
||
| return config; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process of creating ClientConfiguration is made separated from the S3NotebookRepo constructor for making the future modification of this process easier than defining it in the S3NotebookRepo constructor.
|
Thanks @kjmrknsn for the contribution! Could you also update https://github.com/apache/zeppelin/blob/master/docs/setup/operation/configuration.md#zeppelin-properties ? |
b1ec41a to
c66cb63
Compare
|
@Leemoonsoo Thanks for your review. I added the property of (Commits were squashed.) |
|
CI failure looks unrelated to this. LGTM and merge to master if no further review. |
What is this PR for?
Make Zeppelin support Ceph as a notebook storage.
Ceph has APIs which are compatible with AWS S3 APIs. However, it supports only AWS Signature V2 and GetObject requests of aws-sdk-java use V4 by default: aws/aws-sdk-java#372
According to aws/aws-sdk-java#372 (comment) , the Zeppelin configuration of
zeppelin.notebook.s3.signerOverrideis added to make thesignerOverridefield of aClientConfigurationinstance configurable.What type of PR is it?
[Improvement]
Todos
What is the Jira issue?
How should this be tested?
Tested manually.
Screenshots (if appropriate)
Questions: