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.

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 Pub/Sub snapshot and seek functionality #3303

Merged
merged 4 commits into from
Apr 19, 2017

Conversation

geigerj
Copy link
Contributor

@geigerj geigerj commented Apr 17, 2017

Notes:

  • cloud.google.com links do not yet work
  • SeekResponse exists but is empty; the corresponding methods return
    None instead
  • It's not documented whether the deleted topic path applies to
    snapshots analogous to how it applies to subscriptions, but it seems
    logical to assume so.
  • The new Subscription fields are part of this release but will come
    shortly as a separate PR. (They can be done independently of the
    snap-seek work.)

Notes:
- cloud.google.com links do not yet work
- SeekResponse exists but is empty, so corresponding methods return
  None instead
- It's not documented whether the deleted topic path applies to
  snapshots analogous to how it applies to subscriptions, but it seems
  logical to assume so.
- The new Subscription fields are part of this release but will come
  shortly as a separate PR. (They can be done independently of the
  snap-seek work.)
@geigerj geigerj requested a review from lukesneeringer April 17, 2017 05:28
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 17, 2017
@geigerj
Copy link
Contributor Author

geigerj commented Apr 17, 2017

@lukesneeringer CI is failing because it thinks this branch doesn't have test coverage. There is a actually a test intended to exercise this branch. I checked that raising an exception in the allegedly uncovered lines of code causes the test to fail, so not sure exactly why codecov doesn't like this.

@geigerj geigerj requested a review from dhermes April 18, 2017 17:15
@tseaver tseaver added api: pubsub Issues related to the Pub/Sub API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Apr 18, 2017
@lukesneeringer
Copy link
Contributor

This looks fine other than figuring out the CI failure. I can look at that first thing tomorrow morning.

@lukesneeringer
Copy link
Contributor

The reason you are not getting full coverage is because of the if/elif:

if time is not None:
    data['time'] = time
elif snapshot is not None:
    data['snapshot'] = snapshot

You have a test for time being set, and one for snapshot being set, but the only way that the elif will evaluate to False is if neither is set.

I am assuming that setting neither option is an error case. (I assume this is also true if you set both.)

Your current logic basically makes time win over snapshot if both are set. This is probably arbitrary. I suggest making both clauses simple if statements and letting the API itself handle the validation if both (or neither) is set. That will also cause all branches to be covered with your current tests.

Copy link
Contributor

@lukesneeringer lukesneeringer left a comment

Choose a reason for hiding this comment

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

Approved, but make sure you agree with my changes.

@geigerj
Copy link
Contributor Author

geigerj commented Apr 19, 2017

Thanks for figuring this out, @lukesneeringer. That makes sense, and I agree with your changes.

@geigerj geigerj merged commit 5fa507f into googleapis:master Apr 19, 2017
@geigerj geigerj deleted the snap-seek2 branch April 19, 2017 16:02
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
Add Pub/Sub snapshot and seek functionality
@tseaver
Copy link
Contributor

tseaver commented May 19, 2017

@geigerj, @lukesneeringer I'm curious: why did we rush in support for a feature which is not yet public (i.e., it requires whitelisting, has no public docs, etc., even a month later).

@lukesneeringer
Copy link
Contributor

Because it was pushed for by the Pub/Sub team. They claimed at the time that support was imminent. (I raised the non-public issue and was told we were making an exception in this case.)

@tseaver
Copy link
Contributor

tseaver commented May 19, 2017

@lukesneeringer Seems like your concern was well-founded. I think our normal protocol (develop and review the client-side support in a private fork, push to public at launch) is more sensible.

@lukesneeringer
Copy link
Contributor

For features like this, I would prefer not develop them at all until they go public. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants