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

Added per-computation map transform->state_family to windmill's GetConfigResponse. #7566

Merged
merged 6 commits into from
Jan 29, 2019
Merged

Added per-computation map transform->state_family to windmill's GetConfigResponse. #7566

merged 6 commits into from
Jan 29, 2019

Conversation

drieber
Copy link
Contributor

@drieber drieber commented Jan 18, 2019

Added per-computation map transform->state_family to windmill's GetConfigResponse.

Added field computation_config_map to GetConfigResponse. It maps from computation_id to ComputationConfig. ComputationConfig has transform_user_name_to_state_family. This is necessary because in some optimized graphs, the same stateful transform appears in more than one stage, and each instance must use its own state_family.

@drieber
Added fields to GetConfigResponse: computation_config_map maps from c…
e7601b0 
…omputation_id to ComputationConfig. ComputationConfig has transform_user_name_to_state_family. This is necessary because in some optimized graphs, the same stateful transform appears in more than one stage, and each instance must use its own state_family.
@drieber
Copy link
Contributor Author

drieber commented Jan 18, 2019 •
edited
Loading

@dpmills

@dpmills
Copy link
Contributor

dpmills commented Jan 18, 2019

LGTM

@drieber
Copy link
Contributor Author

drieber commented Jan 18, 2019

@kennknowles

@drieber
Copy link
Contributor Author

drieber commented Jan 18, 2019

retest this please

@drieber
Copy link
Contributor Author

drieber commented Jan 19, 2019

retest this please

@drieber
Copy link
Contributor Author

drieber commented Jan 24, 2019

R: @youngoli
R: @kennknowles

youngoli
youngoli approved these changes Jan 25, 2019
View reviewed changes
Copy link
Contributor

@youngoli youngoli 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.

@kennknowles kennknowles self-requested a review January 25, 2019 19:14
@drieber
Copy link
Contributor Author

drieber commented Jan 26, 2019

The error is unrelated:

11:08:06 > Task :beam-runners-core-construction-java:test
11:08:06 Error occurred during initialization of VM
11:08:06 java.lang.OutOfMemoryError: unable to create new native thread
11:08:06 Error occurred during initialization of VM
11:08:06 java.lang.OutOfMemoryError: unable to create new native thread
11:08:06 Process 'Gradle Test Executor 13' finished with non-zero exit value 1

@drieber
Copy link
Contributor Author

drieber commented Jan 29, 2019

retest this please

@kennknowles
Copy link
Member

LGTM. If the Python test fails in a way that looks flaky you can re-run just that one via the words in quotes: "Run Python PreCommit" (not case-sensitive)

@drieber
Copy link
Contributor Author

drieber commented Jan 29, 2019

@kennknowles What are the next steps here? Sorry I am not too familiar with the process yet.

@kennknowles
Copy link
Member

You are g2g. Thanks!

@kennknowles
Copy link
Member

It looks like maybe you have two meaningful commits and a bunch of merge commits. Do you intend that, or are they actually just one to be squashed?

@drieber
Copy link
Contributor Author

drieber commented Jan 29, 2019

I think it is just one to be squashed. Should I do something on my end?

@kennknowles kennknowles merged commit 2daeb8c into apache:master Jan 29, 2019
@kennknowles
Copy link
Member

Nope! Squashed & merged.

wscheep pushed a commit to wscheep/beam that referenced this pull request Feb 2, 2019
@drieber
Added per-computation map transform->state_family to windmill's GetCo…
d3111fd 
…nfigResponse. (apache#7566)

* Added fields to GetConfigResponse: computation_config_map maps from computation_id to ComputationConfig. ComputationConfig has transform_user_name_to_state_family. This is necessary because in some optimized graphs, the same stateful transform appears in more than one stage, and each instance must use its own state_family.

* Fixed the tag of the computation_config field.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngoli youngoli youngoli approved these changes

@kennknowles kennknowles Awaiting requested review from kennknowles

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
@drieber @dpmills @kennknowles @youngoli