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
[BEAM-4610] Add SSL support for RedisIO #8161
Conversation
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.
Nice, some really minor changes to be done and we will be ok to go. Thanks @EdgarLGB !
@@ -79,6 +79,16 @@ | |||
* | |||
* }</pre> | |||
* | |||
* <p>It's also possible to enable Redis SSL connection with the corresponding methods: |
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.
remove the comment (let it like it was)
@@ -99,9 +105,18 @@ public RedisConnectionConfiguration withTimeout(int timeout) { | |||
return builder().setTimeout(timeout).build(); | |||
} | |||
|
|||
/** | |||
* Enable SSL connection to Redis server. |
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.
nit: remove the @return part and inline like the other methods
@@ -199,6 +209,10 @@ public Read withBatchSize(int batchSize) { | |||
return builder().setBatchSize(batchSize).build(); | |||
} | |||
|
|||
public Read enableSSL() { |
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.
Remove, users wanting to do this can do it via the ConnectionConfiguration
object.
} | ||
|
||
@Test | ||
public void testReadBuildsWithSslCorrectly() { |
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.
remove, we may consider a future PR to add ConnectionConfigurationTest
but so far we have avoided to test AutoValue's configurations.
Thanks @iemejia for the code review! FYI, the modification has been done and it is ready to get merged. |
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.
LGTM. Waitng for Jenkins to merge.
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.
LGTM
Hi all,
Here is the PR for SSL connection support to RedisIO. The idea is to just add a SSL flag to allow the Redis client to open a SSL connection with its default SSL parameters. Thanks in advance for the review. (R: @iemejia )
Regards,
Guobao
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.