Skip to content

Conversation

@swegner
Copy link
Contributor

@swegner swegner commented May 11, 2016

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

The DataflowRunner (like other runners) transforms pipelines during construction and sends only primitive transforms to its service. This PR validates that primitive transforms are correctly registering display data for IO transforms.

@swegner
Copy link
Contributor Author

swegner commented May 11, 2016

R: @bjchambers

@swegner swegner force-pushed the displaydata-dataflow branch from df8b28d to 9cfa061 Compare May 11, 2016 17:49
@swegner
Copy link
Contributor Author

swegner commented May 12, 2016

@bjchambers is out-of-office, @lukecwik can you take a look?

@swegner
Copy link
Contributor Author

swegner commented May 12, 2016

/cc @tgroh

DisplayData displayData = DisplayData.from(write);

assertThat(displayData, hasDisplayItem("tableId", "fooTable"));
// BigtableIO adds user-agent to options; assert only on key and not value.
Copy link
Member

Choose a reason for hiding this comment

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

Your comments are out of sync

@swegner
Copy link
Contributor Author

swegner commented May 14, 2016

I've addressed all feedback so far. Please take another look. @tgroh

<artifactId>java-sdk-all</artifactId>
</dependency>

<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this validation required adding dependencies on avro and datastore. Is there a reason these dependencies appeared now but not before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously there was no direct dependency on Avro/Datastore in Dataflow runner. This change adds integration tests which directly reference classes in these packages.

@bjchambers
Copy link
Contributor

LGTM after we confirm the new dependencies and get the build passing.

@swegner
Copy link
Contributor Author

swegner commented May 16, 2016

@bjchambers Travis is green, Jenkins failure looks like a flake.

@swegner swegner force-pushed the displaydata-dataflow branch from d9ac1a6 to 6269b8f Compare May 16, 2016 16:42
@swegner
Copy link
Contributor Author

swegner commented May 16, 2016

@bjchambers squashed. I don't know why Jenkins is still angry, but this is rebased on top of current HEAD.

@asfgit asfgit merged commit 6269b8f into apache:master May 16, 2016
asfgit pushed a commit that referenced this pull request May 16, 2016
@swegner swegner deleted the displaydata-dataflow branch May 16, 2016 18:58
@swegner
Copy link
Contributor Author

swegner commented May 16, 2016

Backport: GoogleCloudPlatform/DataflowJavaSDK#230

dhalperi pushed a commit to dhalperi/beam that referenced this pull request Aug 23, 2016
Implement getSplitPointsConsumed() in BigtableIO
iemejia pushed a commit to iemejia/beam that referenced this pull request Jan 12, 2018
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
Co-authored-by: Craig Labenz <craiglabenz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants