-
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
Refactor functions to use Sink interface #1708
Conversation
@sijie @srkukarni please review. |
CompletableFuture<Void> write(T value) throws Exception; |
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.
Exception should be thrown through CompletableFuture<Void>
, no?
It is a bit strange to throw exception for a method that returns CompletableFuture
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.
+1 on this
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 agree. I will change
void close() throws Exception; | ||
} | ||
|
||
private class PulsarSinkAtMostOnceProcessor implements PulsarSinkProcessor { |
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.
private static
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.
this cannot be static. I am using the PulsarClient and PulsarSinkConfig from the parent class
} | ||
} | ||
|
||
private class PulsarSinkAtLeastOnceProcessor implements PulsarSinkProcessor { |
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.
private static
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.
this cannot be static. I am using the PulsarClient and PulsarSinkConfig from the parent class
|
||
// currently on PulsarRecord | ||
Producer producer = outputProducer.getProducer(pulsarRecord.getTopicName(), | ||
Integer.parseInt(pulsarRecord.getPartitionId())); |
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 think we can change MultiConsumerOneOutputTopicProducers
to use Map<String, Producer>
, rather than converting string back to int
|
||
@Override | ||
public CompletableFuture<Void> write(T value) throws Exception { | ||
return null; |
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.
throw new UnsupportedException(..)
?
} | ||
|
||
@Override | ||
public void close() throws Exception { |
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 think we need to close the producers here, right?
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.
good catch will add
@@ -29,7 +32,9 @@ | |||
* <p>There is a default implementation provided for wrapping up the user provided {@link Sink}. Pulsar sink | |||
* should be implemented using this interface to ensure supporting effective-once. | |||
*/ | |||
public interface RuntimeSink<T> extends Sink<T> { | |||
//public interface RuntimeSink<T> extends Sink<T> { |
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.
the changes here are not needed?
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.
yup will remove
No description provided.