Skip to content

Connect to Cassandra cluster using SSL#14950

Merged
highker merged 1 commit intoprestodb:masterfrom
SandishKumarHN:master
Aug 6, 2020
Merged

Connect to Cassandra cluster using SSL#14950
highker merged 1 commit intoprestodb:masterfrom
SandishKumarHN:master

Conversation

@SandishKumarHN
Copy link
Contributor

@SandishKumarHN SandishKumarHN commented Aug 3, 2020

Connect to Cassandra cluster with tls security

== RELEASE NOTES ==

Cassandra Changes
* Add TLS security support.

@dborkar
Copy link

dborkar commented Aug 5, 2020

@SandishKumarHN thank you for your contribution! This is something community has brought up. Can you share your testing approach?
@highker Can you please take a look?

@highker highker self-requested a review August 6, 2020 06:25
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Minor comments only

Copy link

Choose a reason for hiding this comment

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

static import loadKeyStore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@highker I did not understand this comment. PemReader has all static methods.

Copy link

Choose a reason for hiding this comment

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

same

Copy link

Choose a reason for hiding this comment

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

import static com.facebook.airlift.security.pem.PemReader.readCertificateChain;

Comment on lines 168 to 169
Copy link

Choose a reason for hiding this comment

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

break a line between these two. Also, the indentation doesn't look right.

Comment on lines 25 to 27
Copy link

Choose a reason for hiding this comment

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

one error code per line

@highker highker self-assigned this Aug 6, 2020
@SandishKumarHN
Copy link
Contributor Author

@highker thanks for the review, I have made changes based on your review and left a question. looks like the CI failure is not related to the PR.

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Could you squash your commits into one?

Copy link

Choose a reason for hiding this comment

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

import static com.facebook.airlift.security.pem.PemReader.readCertificateChain;

@SandishKumarHN
Copy link
Contributor Author

@highker thanks for the review again, made changes for static import, and squshed into one commit. CI failure is not related to this PR.

@highker highker merged commit 1d52c57 into prestodb:master Aug 6, 2020
@caithagoras caithagoras mentioned this pull request Aug 14, 2020
7 tasks
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.

3 participants