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

Revert "Re-enable bundling (for consideration)" #1954

Merged
merged 1 commit into from
Jul 8, 2016

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 1, 2016

Reverts #1950

Apparently, we have a much tinier fix possible.

@tseaver tseaver added api: pubsub Issues related to the Pub/Sub API. grpc labels Jul 1, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 1, 2016
@daspecster
Copy link
Contributor

Ah, this still has to be confirmed right?

@tseaver
Copy link
Contributor Author

tseaver commented Jul 1, 2016

Yup. But I wanted to get ready, as my memory of something @geigerj said a week ago makes me think we're going to back it out.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 5, 2016

@geigerj, @bjwatson

        options = CallOptions(is_bundling=True)
        message_pbs = [_message_pb_from_dict(message)
                       for message in messages]
        try:
            result = self._gax_api.publish(topic_path, message_pbs,
                                           options=options)
        except GaxError as exc:
            if exc_to_code(exc.cause) == StatusCode.NOT_FOUND:
                raise NotFound(topic_path)
            raise
        return result.message_ids

fails because the GAX publish wrapper returns an Event object when is_bundling is set True:

======================================================================
ERROR: test_message_pull_mode_e2e (pubsub.TestPubsub)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/system_tests/pubsub.py", line 157, in test_message_pull_mode_e2e
    topic.publish(MESSAGE_1, extra=EXTRA_1)
  File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/gcloud/pubsub/topic.py", line 246, in publish
    message_ids = api.topic_publish(self.full_name, [message_data])
  File "/home/tseaver/projects/agendaless/Google/src/gcloud-python/gcloud/pubsub/_gax.py", line 175, in topic_publish
    return result.message_ids
AttributeError: 'Event' object has no attribute 'message_ids'

@tseaver
Copy link
Contributor Author

tseaver commented Jul 8, 2016

@bjwatson I believe we actually want _gax._PublisherAPI.topic_publish to disable bundling: it needs to remain semantically equivalent to the JSON-over-API version (i.e, returning the message_ids), which makes the async bundling bit less useful.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 8, 2016

@daspecster I think we should go ahead and merge the revert.

@daspecster
Copy link
Contributor

@tseaver ok, LGTM!

@tseaver tseaver merged commit 6b4704b into master Jul 8, 2016
@tseaver tseaver deleted the revert-1950-enable-pubsub-bundling branch July 8, 2016 18:38
This was referenced Aug 3, 2016
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. grpc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants