pulsar connector accepts auth params#22028
Conversation
|
Can one of the admins verify this patch? |
4 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
johnjcasey
left a comment
There was a problem hiding this comment.
Do we handle bad auth in pulsar correctly? i.e. if we pass nulls to a pulsar that requires auth, or pass auth to a pulsar that doesn't?
|
|
||
| // Open connection to Pulsar clients | ||
| @Setup | ||
| // TODO add auth related |
I don't think it is being handled the bad auth scenario but we could add a pre-validation for the auth like in SpannerIO to validate if contains the correct parameters. |
aromanenko-dev
left a comment
There was a problem hiding this comment.
Thanks! I left a couple of comments
| MessageId endMessageId, | ||
| String clientUrl, | ||
| String adminUrl) { | ||
| String adminUrl, |
There was a problem hiding this comment.
Since this is a public method then it would be better to provide another an overloaded method with the same name and additional arguments.
If this class and method are not supposed to be used by user then it should be private or package private to avoid potential breaking changes like this.
| dofnInstance.getInitialRestriction( | ||
| PulsarSourceDescriptor.of( | ||
| TOPIC, expectedStartOffset, null, null, SERVICE_URL, ADMIN_URL)); | ||
| TOPIC, expectedStartOffset, null, null, SERVICE_URL, ADMIN_URL, null, null)); |
There was a problem hiding this comment.
Please, add the tests with non-null auth values.
|
What is the status of this PR? |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
|
added this to [Parent issue] Support for Apache Pulsar #31078 |
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.