-
Notifications
You must be signed in to change notification settings - Fork 966
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 disk size as Dataflow Job Configuration #841
Add disk size as Dataflow Job Configuration #841
Conversation
@@ -38,10 +38,12 @@ public void shouldConvertToPipelineArgs() throws IllegalAccessException { | |||
.setNetwork("default") | |||
.setSubnetwork("regions/asia-east1/subnetworks/mysubnetwork") | |||
.setMaxNumWorkers(1) | |||
.setServiceAccount("serviceaccount") |
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 is service account being modified here? Is this a mistake?
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.
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.
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.
Its still not clear if its necessary or not.
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.
I have removed the service account argument from the test.
protos/feast/core/Runner.proto
Outdated
/* Labels to apply to the dataflow job */ | ||
map<string, string> labels = 13; | ||
map<string, string> labels = 14; |
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 are the proto field numbers being changed?
f155ddf
to
75d89dd
Compare
75d89dd
to
6a0d43f
Compare
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
/test test-end-to-end-batch |
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?: