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.

Sign up for GitHub

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

Jump to bottom

pulsar connector accepts auth params #22028

Closed

nlu90 wants to merge 2 commits into apache:master from nlu90:neng/pulsar-auth

Member

nlu90 commented Jun 23, 2022

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:

Choose reviewer(s) and mention them in a comment (R: @username).
Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

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.


          pulsar connector accepts auth params

9f3a291

asf-ci commented Jun 23, 2022

Can one of the admins verify this patch?

4 similar comments

asf-ci commented Jun 23, 2022

Can one of the admins verify this patch?

asf-ci commented Jun 23, 2022

Can one of the admins verify this patch?

asf-ci commented Jun 23, 2022

Can one of the admins verify this patch?

asf-ci commented Jun 23, 2022

Can one of the admins verify this patch?

nlu90 mentioned this pull request

[Feature Request]: Pulsar Connector should accept auth parameters to connect #22027

Open

github-actions bot added io java labels

johnjcasey reviewed

View reviewed changes

Contributor

johnjcasey left a 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?

sdks/java/io/pulsar/src/main/java/org/apache/beam/sdk/io/pulsar/ReadFromPulsarDoFn.java

                   this.pulsarClientSerializableFunction = transform.getPulsarClient();
                 }
                 // Open connection to Pulsar clients
                 @Setup
+                // TODO add auth related

Contributor

johnjcasey Jun 24, 2022

What is TODO here?

Contributor

MarcoRob commented Jun 24, 2022 •

edited

Loading

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?

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 reviewed

View reviewed changes

Contributor

aromanenko-dev left a comment

Thanks! I left a couple of comments

sdks/java/io/pulsar/src/main/java/org/apache/beam/sdk/io/pulsar/PulsarSourceDescriptor.java

                 public static PulsarSourceDescriptor of(
                     String topic,
                     Long startOffsetTimestamp,
                     Long endOffsetTimestamp,
                     MessageId endMessageId,
                     String clientUrl,
-                    String adminUrl) {
+                    String adminUrl,

Contributor

aromanenko-dev Jun 24, 2022

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.

sdks/java/io/pulsar/src/test/java/org/apache/beam/sdk/io/pulsar/ReadFromPulsarDoFnTest.java

                   OffsetRange result =
                       dofnInstance.getInitialRestriction(
                           PulsarSourceDescriptor.of(
-                              TOPIC, expectedStartOffset, null, null, SERVICE_URL, ADMIN_URL));
+                              TOPIC, expectedStartOffset, null, null, SERVICE_URL, ADMIN_URL, null, null));

Contributor

aromanenko-dev Jun 24, 2022

Please, add the tests with non-null auth values.

Contributor

robertwb commented Jul 15, 2022

What is the status of this PR?


          format and type fix

Contributor

github-actions bot commented Jan 30, 2023

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 [email protected] list. Thank you for your contributions.

github-actions bot added the stale label

Contributor

github-actions bot commented Mar 2, 2023

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.

github-actions bot closed this

hpvd mentioned this pull request

[Parent issue] Support for Apache Pulsar #31078

Open

hpvd commented Apr 24, 2024

added this to [Parent issue] Support for Apache Pulsar #31078

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels