-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[improve][fn] PIP-237: Expose PulsarAdmin in SinkContext and SourceContext #19679
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
base: master
Are you sure you want to change the base?
Conversation
| * @return The instance of pulsar admin client | ||
| */ | ||
| default PulsarAdmin getPulsarAdmin() { | ||
| throw new UnsupportedOperationException("not implemented"); |
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.
what about "Not available in this context" ?
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.
I wanted to keep it consistent with the messages for e.g. getPulsarClient but I'm open to changing too
eolivelli
left a comment
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
what about adding some unit tests ? in order to verify that we don't break compatibility in the future ?
@eolivelli To clarify, with unit tests you mean a test that e.g. calls |
|
@eolivelli Let's get this merged :) |
|
The pr had no activity for 30 days, mark with Stale label. |
Codecov Report
@@ Coverage Diff @@
## master #19679 +/- ##
=============================================
+ Coverage 25.24% 68.07% +42.82%
- Complexity 205 10261 +10056
=============================================
Files 1668 1880 +212
Lines 126201 162952 +36751
Branches 13776 21088 +7312
=============================================
+ Hits 31858 110922 +79064
+ Misses 89386 42676 -46710
- Partials 4957 9354 +4397
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
The pr had no activity for 30 days, mark with Stale label. |
PIP: #19123
Motivation
We want to expose PulsarAdmin in the connector context's just as we do for the functions context.
Modifications
Moved the method out of the functions
Contextinto theBaseContextinterface.Verifying this change
This change is already covered by existing tests, such as (please describe tests).
ContextImplTestDoes this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: alpreu#8