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

Using Iterators for list_topics() in Pub/Sub. #2602

Merged
merged 2 commits into from
Oct 25, 2016
Merged

Using Iterators for list_topics() in Pub/Sub. #2602

merged 2 commits into from
Oct 25, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 25, 2016 •
edited
Loading

In the process, had to add custom support for the GAX page iterator in our core Iterator implementation.

@tseaver @jonparrott I am sending this out as-is hoping for some feedback (list_subscriptions and topic_list_subscriptions also need to be done). I don't feel that great about the way I did custom paging in Iterator, maybe it's worth defining a GAX subclass of Iterator?

Also, in order to avoid too many radical changes to the code, I punted on getting a client into the Iterator returned by the _gax / connection implementations. As can be seen by the unit tests, this effectively makes them useless (more-so in the HTTP case than in the GAX case). I'd be happy to send a follow-up PR to make client accessible in the _PublisherAPI implementation, but again, wanted to make this commit digestible.

NOTE: Has #2594 as diffbase.

@dhermes dhermes added the api: pubsub Issues related to the Pub/Sub API. label Oct 25, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 25, 2016
tseaver
tseaver reviewed Oct 25, 2016
View reviewed changes
core/google/cloud/_testing.py
raise StopIteration
else:
items = self._items
self._items = None

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

core/google/cloud/iterator.py
:class:`Iterator` that started the page. By default uses
the HTTP pages iterator. Meant to provide a custom
way to create pages (potentially with a custom
transport such as gRPC).

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub/_gax.py
# iterator instance returned.
return Iterator(client=None, path=path,
item_to_value=_item_to_topic,
page_iter=page_iter)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

pubsub/google/cloud/pubsub/connection.py
@@ -104,7 +106,8 @@ class _PublisherAPI(object):
def __init__(self, connection):
self._connection = connection

def list_topics(self, project, page_size=None, page_token=None):
@staticmethod
def list_topics(project, page_size=None, page_token=None):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

system_tests/pubsub.py
@@ -77,7 +89,7 @@ def test_create_topic(self):
self.assertEqual(topic.name, topic_name)

def test_list_topics(self):
before, _ = Config.CLIENT.list_topics()
before = list(Config.CLIENT.list_topics())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Oct 25, 2016

Can we disable the Circle CI bit until we fix its config? Issues with it:

  • The bit which skips system-tests for PRs is Travis-specific.
  • It is barfing trying to find the correct Python 3.4 / 3.5 interpreter.

@dhermes
Using Iterators for list_topics() in Pub/Sub.
83beb2c 
In the process, had to add custom support for the GAX
page iterator in our core Iterator implementation.
@dhermes dhermes mentioned this pull request Oct 25, 2016
Closed
@dhermes
Passing client to Pub/Sub API classes.
95bdd83 
Also tweaking an exhausted _GAXPageIterator in
google.cloud._testing.
@tseaver
Copy link
Contributor

tseaver commented Oct 25, 2016

LGTM with the _client attr on the API objects.

@dhermes
Copy link
Contributor Author

dhermes commented Oct 25, 2016

@tseaver So I am good to merge? I am currently in the process of breaking Iterator up into Iterator (a base), HTTPIterator and GAXIterator. I think that'd be better as a standalone PR.

As for the Travis failure, that is due to a merge of RuntimeConfig conflicting with #2594 (which is totally fine, I'll send a PR to fix ASAP).

tseaver
tseaver approved these changes Oct 25, 2016
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

@dhermes Yes, the "LGTM" was sincere.

@dhermes
Copy link
Contributor Author

dhermes commented Oct 25, 2016

@tseaver Haha thanks. (I wasn't sure if you were LGTM-ing the PR or just the single commit, pending work to be done.)

I'll make sure to get the refactor out soon.

@dhermes dhermes merged commit 0e4a660 into googleapis:master Oct 25, 2016
@dhermes dhermes deleted the pubsub-iterators branch October 25, 2016 18:57
@dhermes dhermes mentioned this pull request Nov 14, 2016
Merged
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull request googleapis#2602 from dhermes/pubsub-iterators
cbd5c20 
Using Iterators for list_topics() in Pub/Sub.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver approved these changes

Assignees

@tseaver tseaver

@theacodes theacodes

Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhermes @tseaver @daspecster @theacodes @googlebot