-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: add the pubsublite sink #21
Conversation
} | ||
|
||
@Override | ||
public Message serialize(T value, Instant timestamp) { |
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.
should this be used as the event timestamp field?
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.
Oh, yeah, good point! Done (in the serialize pr)
|
||
@Override | ||
public Message serialize(T value, Instant timestamp) { | ||
return Message.fromProto( |
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.
Nit, use the builder methods on Message
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.
Done
import java.io.Serializable; | ||
|
||
@AutoValue | ||
public abstract class PubsubLiteSinkSettings<IN> implements Serializable { |
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.
Use InputT
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.
Done
6a7f71e
to
afa3ca5
Compare
afa3ca5
to
48ea653
Compare
<dependency> | ||
<groupId>org.apache.flink</groupId> | ||
<artifactId>flink-runtime_2.11</artifactId> | ||
<version>1.13.0</version> |
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.
nit. add a property "flink.version" and use it everywhere you have 1.13.0. See pubsublite-beam-io for an example.
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.
Will do in another pr, tianzi suggested that I do this for pubsublite too
|
||
@Override | ||
public synchronized void snapshotState( | ||
org.apache.flink.runtime.state.FunctionSnapshotContext functionSnapshotContext) |
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.
Why not import these?
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.
Done. Intellij generated them like this and I never changed them
public abstract Builder<InputT> setTopicPath(TopicPath value); | ||
|
||
abstract Builder<InputT> setSerializationSchema(PubsubLiteSerializationSchema<InputT> value); | ||
|
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.
is this the proper user interface? If so, add comments.
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.
Added comments.
return new AutoValue_PubsubLiteSinkSettings.Builder<InputT>().setSerializationSchema(schema); | ||
} | ||
|
||
public static PubsubLiteSinkSettings.Builder<Message> messagesBuilder() { |
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.
Just Builder
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.
Done, better, thanks
|
||
@AutoValue | ||
public abstract class PubsubLiteSinkSettings<InputT> implements Serializable { | ||
public static <InputT> PubsubLiteSinkSettings.Builder<InputT> builder( |
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.
just Builder
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.
Done
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️