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.

Sign up for GitHub

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

Jump to bottom

KafkaIO implementation for feast #19

Merged
merged 2 commits into from
Dec 28, 2018
Merged

KafkaIO implementation for feast #19

merged 2 commits into from
Dec 28, 2018

Conversation

baskaranz
Copy link
Contributor

  • Support for reading message from kafka topics as FeatureRow using Apache Beam's KafkaIO implementation
  • Profiles to building ingestion jar for specific runners (flink, dataflow and direct)
  • Minor refactoring

zhilingc
zhilingc reviewed Dec 28, 2018
View reviewed changes
Copy link
Collaborator

@zhilingc zhilingc 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 to me :)

woop
woop approved these changes Dec 28, 2018
View reviewed changes
Copy link
Member

@woop woop left a comment

Choose a reason for hiding this comment

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

LGTM

ingestion/src/main/java/feast/ingestion/ImportJob.java
} catch (IOException e) {
log.error("Exception while setting gcp credential manually : ", e.getMessage());
}
log.info("options: " + options.toString());
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't print out the credentials, right?

@woop woop merged commit 73a0290 into feast-dev:master Dec 28, 2018
@tims
Copy link
Contributor

tims commented Dec 31, 2018

This looks mostly good, but I don't think this should have been merged so quickly.
Are we okay with making pull requests without issues attached?

  • 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.

I'd suggest we revert this and fix the build at least.

@tims tims mentioned this pull request Dec 31, 2018
Closed
Yanson pushed a commit to Yanson/feast that referenced this pull request Jul 29, 2020
@ff-mriko-morandi
Merge pull request feast-dev#19 from farfetch-external/databricks_job…
b999861 
…_manager

Databricks job manager
Yanson pushed a commit to Yanson/feast that referenced this pull request Jul 29, 2020
@algattik
Terraform CI/CD env (feast-dev#25)
dad9ffe 
Closes KE-712, KE-738

Deploys an IaC environment with Feast and Databricks, and runs e2e tests with Redis (using DirectRunner for now, as tests with Databricks runner depend on PR feast-dev#19 and currently fail with timeout issues)

Deletes infra/charts/feast/charts/feast-serving/templates/configmap-store.yaml (To be reviewed)Closes KE-712, KE-738

Deploys an IaC environment with Feast and Databricks, and runs e2e tests with Redis (using DirectRunner for now, as tests with Databricks runner depend on PR feast-dev#19 and currently fail with timeout issues)

Deletes infra/charts/feast/charts/feast-serving/templates/configmap-store.yaml
dmartinol pushed a commit to dmartinol/feast that referenced this pull request Jun 24, 2024
@redhatHameed
Merge pull request feast-dev#19 from dmartinol/feast-rbac
9ffce65 
Feast RBAC Model
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhilingc zhilingc zhilingc left review comments

@woop woop woop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@baskaranz @tims @woop @zhilingc