-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Issue 10445][pulsar-io] Exposed SubscriptionType in the SinkContext #10446
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.
overall is good.
I would also add a (unit, non integration) test in order to ensure that the Sink receives the value.
pulsar-io/core/src/main/java/org/apache/pulsar/io/core/ConnectorContext.java
Outdated
Show resolved
Hide resolved
pulsar-io/core/src/main/java/org/apache/pulsar/io/core/ConnectorContext.java
Outdated
Show resolved
Hide resolved
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
6a422d5
to
1fd766d
Compare
rebased on current master |
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.
before merging this pull request, we should get stakeholders like @jerrypeng and @srkukarni to confirm if this API works for them.
pulsar-io/core/src/main/java/org/apache/pulsar/io/core/SinkContext.java
Outdated
Show resolved
Hide resolved
@jerrypeng @srkukarni Do you have any feedback on this? (I cannot add you to reviewers, no permissions) |
@jerrypeng @srkukarni ping. |
I don't see any red flags in this PR but I also don't understand adding this functionality will be useful. @dlg99 based the comments It seem like the subscription type should be checked at submission time and be rejected if a wrong subscription type is used. If we are checking the subscription type during the sink open, what are we going to do if the subscription type is wrong? Just go into a crash loop? |
@jerrypeng This is not different from what's happening in other connectors now. I agree we can consider adding something to handle it better, but this is out of the scope of this PR. pulsar/pulsar-io/kafka/src/main/java/org/apache/pulsar/io/kafka/KafkaAbstractSink.java Lines 84 to 91 in 5a1264d
pulsar/pulsar-io/kinesis/src/main/java/org/apache/pulsar/io/kinesis/KinesisSink.java Lines 159 to 163 in 5a1264d
etc |
I am not a fan of adding new interfaces to support a not well thought out behavior that might be changed in the future but I don't see a major problem with this as well. |
Thank you @jerrypeng @sijie you left "request changes" status. |
…pache#10446) Fixes apache#10445 ### Motivation SinkContext should expose Subscription type to the Sink More context: apache#9927 (comment) Needed for apache#9927 ### Modifications Added `getSubscriptionType()` to the `SinkContext` interface and `ContextImpl`
Fixes #10445
Motivation
SinkContext should expose Subscription type to the Sink
More context: #9927 (comment)
Needed for #9927
Modifications
Added
getSubscriptionType()
to theSinkContext
interface andContextImpl
Verifying this change
This change is already covered by existing unit tests
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesNew method added to the interface that is a public API. Default method implementation is provided.
Documentation