Skip to content
This repository was archived by the owner on Nov 11, 2022. It is now read-only.

Fix InProcessPipelineRunner to handle a null subscription - #547

Merged
tgroh merged 3 commits into
GoogleCloudPlatform:masterfrom
sammcveety:fix_pubsub_inprocess_null_topic
Feb 15, 2017
Merged

Fix InProcessPipelineRunner to handle a null subscription#547
tgroh merged 3 commits into
GoogleCloudPlatform:masterfrom
sammcveety:fix_pubsub_inprocess_null_topic

Conversation

@sammcveety

@sammcveetysammcveety commented Feb 2, 2017

Copy link
Copy Markdown
Contributor

tgroh
tgroh reviewed Feb 6, 2017
View reviewed changes
Comment threadsdk/src/test/java/com/google/cloud/dataflow/sdk/io/PubsubUnboundedSourceTest.java Outdated
}

@Test
public void testNullTopic() throws Exception {

@tgrohtgrohFeb 2, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Null subscription, right? We probably also want to show that we created a subscription.

@sammcveety

sammcveety commented Feb 11, 2017

Copy link
Copy Markdown
ContributorAuthor

For verifying the subscription, is the least-bad way I can see is to make an additional factory method that includes a verifyCreateSubscription toggle. Anything nicer than that?

@dhalperi

dhalperi commented Feb 14, 2017

Copy link
Copy Markdown
Contributor

Is this fixing the same bug as apache/beam@4406414 ?

Thoughts on a more direct backport?

@dhalperi

dhalperi commented Feb 14, 2017

Copy link
Copy Markdown
Contributor

better link: apache/beam#1817

@sammcveety

sammcveety commented Feb 14, 2017 via email

Copy link
Copy Markdown
ContributorAuthor

@tgroh

tgroh commented Feb 14, 2017

Copy link
Copy Markdown
Contributor

I don't think there's anything nicer than that, but I'm not particularly bothered (given that it's a PubsubTestClientFactory)

@sammcveety

sammcveety commented Feb 15, 2017

Copy link
Copy Markdown
ContributorAuthor

Done, please take another look.

tgroh
tgroh approved these changes Feb 15, 2017
View reviewed changes

@tgrohtgroh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tgroh
tgroh merged commit 9b9ee0b into GoogleCloudPlatform:masterFeb 15, 2017
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@tgrohtgrohtgroh approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sammcveety@dhalperi@tgroh@googlebot