Skip to content

Add disk size as Dataflow Job Configuration - #841

Merged
feast-ci-bot merged 1 commit into
feast-dev:masterfrom
khorshuheng:add-disksize-job-config
Jun 30, 2020
Merged

Add disk size as Dataflow Job Configuration#841
feast-ci-bot merged 1 commit into
feast-dev:masterfrom
khorshuheng:add-disksize-job-config

Conversation

@khorshuheng

Copy link
Copy Markdown
Collaborator

What this PR does / why we need it:
After #718, it's not possible to have arbitrary options set for runner configuration. Instead, it has to be one of the options supported by the protos defined in DataflowRunnerConfigOptions .

diskSizeGb is an important option which wasn't available in the proto. This PR added the configuration.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Users will now be able to specify disk size for dataflow workers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is service account being modified here? Is this a mistake?

@khorshuheng khorshuheng Jun 30, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Service account is part of the available configurations, but it was never tested in the test. We can have this in another PR if preferable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Its still not clear if its necessary or not.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have removed the service account argument from the test.

Comment thread protos/feast/core/Runner.proto Outdated

@woop woop Jun 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are the proto field numbers being changed?

@khorshuheng
khorshuheng force-pushed the add-disksize-job-config branch from f155ddf to 75d89dd Compare June 30, 2020 01:25
@khorshuheng
khorshuheng force-pushed the add-disksize-job-config branch from 75d89dd to 6a0d43f Compare June 30, 2020 01:34
@feast-ci-bot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khorshuheng, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop

woop commented Jun 30, 2020

Copy link
Copy Markdown
Member

/lgtm

@khorshuheng

Copy link
Copy Markdown
Collaborator Author

/test test-end-to-end-batch

@feast-ci-bot
feast-ci-bot merged commit cf5ed2d into feast-dev:master Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants