[BEAM-173] Publish DisplayData for PipelineOptions #127
Conversation
|
R: @lukecwik |
| /** | ||
| * A property defined in a {@link PipelineOptions} interface. | ||
| */ | ||
| static class Property { |
There was a problem hiding this comment.
Perhaps this should be a PipelineOption (since it makes sense that a PipelineOptions object contains PipelineOption objects).
There was a problem hiding this comment.
Yeah, I'm not very happy with the current name. I'm weary of introducing PipelineOption as it sounds like a primitive, and I don't want the scope of this to balloon too much. But maybe..
There was a problem hiding this comment.
It would be nice to only have either PropertyDescriptor or Property but not both.
There was a problem hiding this comment.
I looked further into this, and it seems we really have two disjoint concepts that won't merge well together. We're using PropertyDescriptor to represent the combined options surface for all known PipelineOptions interfaces. What we need for DisplayData is information about the specific PipelineOptions interface(s) than an option comes from. PropertyDescription doesn't work well here. I think it's best to keep the two concepts separate.
I've renamed this class to PipelineOptionSpec-- let me know what you think.
swegner
force-pushed
the
displaydata-pipeline
branch
2 times, most recently
from
16802b9 to
7573549
Compare
| @Override | ||
| public void setTempLocation(String tempLocation) { | ||
| } | ||
|
|
There was a problem hiding this comment.
Replace with PipelineOptionsFactory.create() instead of anonymous inner class of PipelineOptions
swegner
force-pushed
the
displaydata-pipeline
branch
3 times, most recently
from
251ef71 to
3769ef4
Compare
|
I've addressed all feedback so far. Please take another look. @lukecwik |
| } | ||
| } | ||
|
|
||
| /** |
| + Arrays.toString(args) + "]."); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
it won't be retrieved from deserialized JSON since that is contained completely within a different map.
This just tracks whether came from a default vs being explicitly set.
|
Last minor fixups and then LGTM |
swegner
force-pushed
the
displaydata-pipeline
branch
from
946d56d to
249aa93
Compare
|
I've addressed all feedback so far. Please take another look. @lukecwik |
|
It looks like you missed two comments about fixing javadoc. |
swegner
force-pushed
the
displaydata-pipeline
branch
from
5c7bcb5 to
45aa73c
Compare
|
Sorry, missed those javadoc comments. this should be ready now. @lukecwik |
|
LGTM |
|
Backported via GoogleCloudPlatform/DataflowJavaSDK#216 |
* feat: create AsyncIter class for mocking * fix: type error on mocked return on batch_get_documents * feat: integrate microgen async client to query
…y-name-cache Introduce CachedToStringMetricKey and CachedToStringMetricName for Improved Performance
3 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
number, if there is one.
Individual Contributor License Agreement.