Skip to content

Adding PulsarAdmin configs to PulsarHelper#158

Merged
nlu90 merged 9 commits intostreamnative:masterfrom
ericm-db:reconnect-comment
Aug 31, 2023
Merged

Adding PulsarAdmin configs to PulsarHelper#158
nlu90 merged 9 commits intostreamnative:masterfrom
ericm-db:reconnect-comment

Conversation

@ericm-db
Copy link
Copy Markdown
Contributor

Motivation

In #151 we added admin.url without adding Admin Confs.

Modifications

Describe the modifications you've done.

Basically reverting #120

Verifying this change

  • [ x] Make sure that the change passes the CI checks.

(Please pick either of the following options)

  • [ x] This change is a trivial rework / code cleanup without any test coverage.

  • This change is already covered by existing tests, such as:

  • This change added tests and can be verified as follows:

Documentation

Check the box below.

Need to update docs?

  • doc-required
  • no-need-doc
  • doc

@ericm-db ericm-db requested review from a team and nlu90 as code owners August 29, 2023 00:56
@github-actions github-actions Bot added the no-need-doc This pr does not need any document label Aug 29, 2023
@ericm-db
Copy link
Copy Markdown
Contributor Author

@chaoqin-li1123 For some reason I can't add you as a reviewer

Comment thread src/main/scala/org/apache/spark/sql/pulsar/PulsarProvider.scala Outdated
Comment thread src/main/scala/org/apache/spark/sql/pulsar/PulsarProvider.scala
@chaoqin-li1123
Copy link
Copy Markdown
Contributor

Code change LGTM, Thanks for the work!
Could you fix the scala style error?
Also please update the doc that admin config need to be provided for admission control to work.

@chaoqin-li1123
Copy link
Copy Markdown
Contributor

Can we add a small test for this change? Set one of the pulsar.admin.* option in the query, if this option is invalid, an exception should be thrown.

Copy link
Copy Markdown
Contributor

@chaoqin-li1123 chaoqin-li1123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@nlu90 nlu90 merged commit 3f2e904 into streamnative:master Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-need-doc This pr does not need any document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants