samples: add samples #37

anguillanneuf · 2020-10-03T01:20:13Z

Testing:

Samples are tested as part of the required test pubsublite-build. They can be also tested using the following commands locally:

export INSTALL_LIBRARY_FROM_SOURCE=true
cd samples/snippets
python -m nox
# Optionally (choose a session): python -m nox -s lint py-3.6

Summary:

This PR includes the following samples and all their tests are in ./quickstart_test.py:

.
├── create_lite_subscription_example.py
├── create_lite_topic_example.py
├── delete_lite_subscription_example.py
├── delete_lite_topic_example.py
├── get_lite_subscription_example.py
├── get_lite_topic_example.py
├── list_lite_subscriptions_in_project_example.py
├── list_lite_subscriptions_in_topic_example.py
├── list_lite_topics_example.py
├── publish_with_batch_settings_example.py
├── publish_with_custom_attributes_example.py
├── publish_with_ordering_key_example.py
├── publisher_example.py
├── subscriber_example.py
├── update_lite_subscription_example.py
└── update_lite_topic_example.py

Note to reviewer:

Please expect this PR to have the do not merge label as we work to polish the library. Below are some issues I have identified the library. Your reviews can be used for support and/or to open additional issues. Update: These issues have been addressed.

~~Admin, publisher, and subscriber client import and creation are atypical. issue: atypical client creation #45~~
~~Publisher futures do not return outside a context manager. issue: publish future hangs when a context manager is not used #46~~
~~make_subscriber() does not return a subscriber client but a streaming pull future. issue: make_subscriber() doesn't make a subscriber #47~~
~~Admin client expects a string but publisher and subscriber clients expect typed objects for resource names. issue: make clients accept strings and typed objects for resource names #48~~
FlowControlSettings in the subscriber example should ideally be created as a type: from google.cloud.pubsublite.types import FlowControlSettings. Refactor FlowControlSettings under google.cloud.pubsublite.types #49
Consider bringing the following under google.cloud.pubsublite.types: a) Classes in google.cloud.pubsublite.location b) Classes in google.cloud.pubsublite.paths c) Classes in google.cloud.pubsublite.endpoints. Refactor paths/location/endpoints under google.cloud.pubsublite.types #50

add create topic set project number lint

samples/snippets/admin.py

samples/snippets/create_lite_subscription_example.py

fix

samples/snippets/create_lite_subscription_example.py

samples/snippets/create_lite_topic_example.py

tmatsuo · 2020-10-16T22:44:16Z

samples/snippets/quickstart_test.py

+    assert "created successfully." in out
+
+
+def test_update_lite_topic_example(topic_path, capsys):


A small concern that this test depends on the execution order. Probably not a big deal.

samples/snippets/quickstart_test.py

* fix: Move types to common directory Also move internal-only files to internal directory. * fix: Fix test imports

tmatsuo · 2020-11-05T21:58:17Z

samples/snippets/create_lite_subscription_example.py

+    subscription_path = SubscriptionPath(project_number, location, subscription_id)
+
+    subscription = Subscription(
+        name=str(subscription_path),


re: str(), it might be a library usability issue, but isn't it better if we can just pass the SubscriptionPath as the argument? Is it possible to do it in the constructor? Feel free to ignore, or file a new issue and fix later.

This type is a proto, so unfortunately not unless we want to wrap the proto.

Ok, so nevermind.

https://github.com/googleapis/python-pubsublite/blob/master/google/cloud/pubsublite/types/paths.py looks like a handwritten file to me.

The generator usually produces methods to construct and decompose paths (see examples in AutoML). https://aip.dev/client-libraries/4231

Did that not happen for pubsublite?

It produces methods but not types. These methods (like

python-pubsublite/google/cloud/pubsublite_v1/services/admin_service/client.py

Line 135 in 7735d2f

def subscription_path(project: str, location: str, subscription: str,) -> str:

) produce strings as their output: meaning there is no way to type check arguments of path type. If these produced wrapper objects I would have used them.

They also don't type check the current restriction (that we're trying to remove in the backend) that only project numbers be used in the path.

Those methods did get generated for pubsublite but they aren't used.

Topic paths are created by a publisher client in Pub/Sub but by an admin client in Pub/Sub Lite.

# This looks cumbersome. client = AdminClient(cloud_region) topic_path = client.topic_path(project_number, location, topic_id) with PublisherClient() as publisher_client: api_future = publisher_client.publish(topic_path, data.encode("utf-8"))

Ah okay, thanks for the explanation. 👍

Was there a decision made that pubsublite is not going to support project IDs? https://google.aip.dev/cloud/2510 I haven't been looking at all these PRs so apologies if this is digging up an old conversation.

Its the difference between "must" and "should". Its nontrivial work to convert between the two and it has not been addressed until this quarter. I've sent you the internal link from the elysium team that clarifies this guidance.

The discussion was in internal b/162623801. Java currently accepts both. Python/Go will wait for this change on the server side.

tmatsuo · 2020-11-05T22:08:29Z

samples/snippets/publish_with_batch_settings_example.py

+            data = f"{message}"
+            api_future = publisher_client.publish(topic_path, data.encode("utf-8"))
+            # result() blocks. To resolve API futures asynchronously, use add_done_callback().
+            message_id = api_future.result()


re: result(), is there a hard deadline, or will it block forever?

result() can take an optional timeout argument, by default, it blocks forever unless it errors out first.

Can we have timeout then? I think we should avoid infinite wait if possible.

It's rare for result() to fail. In the case of failure, the client library will retry publish if no timeout is provided (as is the case for Pub/Sub too). I think not adding timeout is what most people want.

Ok, I can live with that, but keep in mind if a rare case happens, the code will indefinitely wait.

dpcollins-google

LGTM once two comments are addressed.

samples/snippets/create_lite_topic_example.py

samples/snippets/list_lite_subscriptions_in_topic_example.py

lint

d0dca10

add create topic set project number lint

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 3, 2020

dpcollins-google reviewed Oct 5, 2020

View reviewed changes

samples/snippets/admin.py Outdated Show resolved Hide resolved

anguillanneuf added 6 commits October 6, 2020 09:50

construct topic_path using provided methods

3be7e35

reorganize samples and tests

ba696d1

add admin samples for topic

fd574d0

add remaining admin samples

170612a

lint

59e3a35

publisher quickstart

4f224f9

dpcollins-google reviewed Oct 7, 2020

View reviewed changes

samples/snippets/create_lite_subscription_example.py Outdated Show resolved Hide resolved

anguillanneuf added 5 commits October 7, 2020 10:27

Merge branch 'master' into samples-admin

935f3d8

pythonic param passing

cda5a0c

lint

5f7a40e

solve merge conflict

d9fc6c4

add 3 publisher samples

8338f7c

anguillanneuf changed the title ~~samples: add admin samples~~ samples: add samples Oct 9, 2020

anguillanneuf added 5 commits October 9, 2020 09:39

Merge branch 'master' into samples-admin

0db376e

add publish with batch settings

39af923

temp

35066c2

Merge branch 'master' into samples-admin

27f7d4e

subscriber example and lint

9ca10e5

fix

anguillanneuf requested a review from a team October 16, 2020 01:13

anguillanneuf marked this pull request as ready for review October 16, 2020 01:14

anguillanneuf added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 16, 2020

tmatsuo reviewed Oct 16, 2020

View reviewed changes

anguillanneuf and others added 3 commits October 19, 2020 15:05

use objects for resource names at places

d6fc04a

fix: Move types to common directory (#51)

d56c225

* fix: Move types to common directory Also move internal-only files to internal directory. * fix: Fix test imports

import from types

7df1d9e

anguillanneuf requested a review from a team as a code owner October 19, 2020 22:36

merge conflicts

dea4117

update samples to match library updates

6f1f736

anguillanneuf requested review from tmatsuo and dpcollins-google November 5, 2020 21:11

tmatsuo approved these changes Nov 5, 2020

View reviewed changes

tmatsuo added the snippet-bot:force-run Force snippet-bot runs its logic label Nov 5, 2020

snippet-bot bot removed the snippet-bot:force-run Force snippet-bot runs its logic label Nov 5, 2020

anguillanneuf removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 5, 2020

dpcollins-google reviewed Nov 5, 2020

View reviewed changes

samples/snippets/create_lite_topic_example.py Outdated Show resolved Hide resolved

samples/snippets/list_lite_subscriptions_in_topic_example.py Outdated Show resolved Hide resolved

use capacity instead of scale and other nit

3b62376

anguillanneuf requested a review from dpcollins-google November 5, 2020 23:21

dpcollins-google approved these changes Nov 6, 2020

View reviewed changes

anguillanneuf requested a review from busunkim96 November 6, 2020 20:15

anguillanneuf merged commit 7fae567 into master Nov 9, 2020

anguillanneuf deleted the samples-admin branch November 9, 2020 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: add samples #37

samples: add samples #37

anguillanneuf commented Oct 3, 2020 •

edited

tmatsuo Oct 16, 2020

tmatsuo Nov 5, 2020

dpcollins-google Nov 5, 2020

tmatsuo Nov 5, 2020

busunkim96 Nov 6, 2020

dpcollins-google Nov 6, 2020

dpcollins-google Nov 6, 2020

anguillanneuf Nov 6, 2020

busunkim96 Nov 7, 2020

dpcollins-google Nov 7, 2020

anguillanneuf Nov 9, 2020

tmatsuo Nov 5, 2020

anguillanneuf Nov 5, 2020 •

edited

tmatsuo Nov 5, 2020

anguillanneuf Nov 5, 2020

tmatsuo Nov 5, 2020

dpcollins-google left a comment

		assert "created successfully." in out


		def test_update_lite_topic_example(topic_path, capsys):

samples: add samples #37

samples: add samples #37

Conversation

anguillanneuf commented Oct 3, 2020 • edited

Testing:

Summary:

Note to reviewer:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anguillanneuf Nov 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpcollins-google left a comment

Choose a reason for hiding this comment

anguillanneuf commented Oct 3, 2020 •

edited

anguillanneuf Nov 5, 2020 •

edited