Skip to content
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

Add Env as argument to source/sink builders #1734

Merged
merged 1 commit into from Nov 20, 2017
Merged

Add Env as argument to source/sink builders #1734

merged 1 commit into from Nov 20, 2017

Conversation

jtfmumm
Copy link
Contributor

@jtfmumm jtfmumm commented Nov 16, 2017

Particular source and sink implementations (like our
Kafka implementations) might need access to OutStream
for logging purposes. We cannot hold a reference to this
in our builders since then they cannot be serialized to
send across workers during initialization. This threads
Env through the builder apply methods so that we can
provide the reference when we are actually building on
a particular worker.

Closes #1241

Particular source and sink implementations (like our
Kafka implementations) might need access to `OutStream`
for logging purposes. We cannot hold a reference to this
in our builders since then they cannot be serialized to
send across workers during initialization. This threads
Env through the builder apply methods so that we can
provide the reference when we are actually building on
a particular worker.
Copy link
Contributor

@slfritchie slfritchie left a comment

Choose a reason for hiding this comment

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

LGTM. The CI failure is due to another problem that you and Nisan have been chasing?

@jtfmumm
Copy link
Contributor Author

jtfmumm commented Nov 16, 2017

@slfritchie Looks like the same one, yeah

Copy link
Contributor

@dipinhora dipinhora left a comment

Choose a reason for hiding this comment

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

Looks good. I'll make the changes on the kafka source/sink side in the next few days. Thanks.

@jtfmumm jtfmumm merged commit fe522ef into master Nov 20, 2017
wallaroolabs-wally added a commit that referenced this pull request Nov 20, 2017
@jtfmumm jtfmumm deleted the builder-env branch December 8, 2017 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants