Kafka source

[tims]

The kafka IO PR #19 needs some follow ups.

• It appears to have broken the build? Or another merge removed an import. IOException symbol not found in ImportJob.
• Import job gets google application default creds and uses options.setGcpCredential? Why? I've not needed to do that before if I set them in my environment. It would also seems odd to get an error logged about it if you're using the direct or flink runner and not using GCP at all.
• ImportJobOptions should not inherit FlinkPipelineOptions and GcpOptions, as that suggests it should always use Flink and GCP. These should already be registered by default, so you can just make use of them using options.as(GcpOptions.class) if needed.

[tims]

I also feel that the deserializer classes should be either inner classes or in a separate package with the feature row kafka IO class. As they are very kafka specific. But we can just move them when we get around to modularizing all the source classes.

[zhilingc]

I think i accidentally removed the IOException import when I did the merge for PR #12, sorry :(

[tims]

I'm fixing it, pull request incoming

[tims]

@baskaranz can you take a look at this.

Also, can you please use the google-java-format plugin if you're using intellij, we standardised on that.

[tims]

I think we should also write an integration test before we close this issue.

[woop]

I think we should also write an integration test before we close this issue.

I agree. Not having an integration test is part of the challenge. We merge based on trust that all relevant testing was done. Integration tests would remove that problem.

[tims]

@baskaranz can you take on making unit test for the FeatureRowKafkaIO?

[baskaranz]

Tim, I can take it. Thanks, Baskaran

…

On Thu, Jan 3, 2019 at 1:23 PM Tim Sell ***@***.***> wrote: @baskaranz <https://github.com/baskaranz> can you take on making unit test for the FeatureRowKafkaIO? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGBq42gmBuqhwQGKK86rKMX2evO7iXAaks5u_ZO3gaJpZM4ZlMXh> .

[tims]

@baskaranz any updates on this issue?

As discussed in #13. Can you also remove the FeatureRowKey proto message? We don't actually care what the key is, as everything is in the message, so we don't need it when reading. That's a concern for the publisher.

[zhilingc]

One more bug that I'd like to tag to this issue:

On the core side, the import spec validation prevents the submission of kafka jobs.

[woop]

@tims @baskaranz @zhilingc, what is the current status of this issue, are we still pending the following?

• Creation of unit tests for FeatureRowKafkaIO
• Creation of integraton tests for testing KafkaIO
• Removal of FeatureRowKey proto message
• Fixing Import spec validation that prevents submission of Kafka jobs

[baskaranz]

@woop apart from Creation of unit tests for FeatureRowKafkaIO, the rest of the items were taken care since there's an issue running the tests using FeatureRowKafkaIO. I'm working with @pradithya on this. It works anyway, we use it to read from feature stream. We can close this issue for now and open a new issue for Creation of unit tests for FeatureRowKafkaIO.