issue: atypical client creation

[anguillanneuf]

Current:

from google.cloud.pubsublite.make_admin_client import make_admin_client
from google.cloud.pubsublite.cloudpubsub.make_publisher import make_publisher
from google.cloud.pubsublite.cloudpubsub.make_subscriber import make_subscriber

Expected:

from google.cloud.pubsublite import AdminClient
from google.cloud.pubsublite import PublisherClient
from google.cloud.pubsublite import SubscriberClient

[dpcollins-google]

Can you clarify what you want these classes to look like? Is the problem that the method is not named FooClient.__init__? This is impossible to do if AdminClient is the interface being implemented (would be a recursive import), and requires it to be an actual implementation which I would like to avoid.

[anguillanneuf]

python-pubsub decorates handwritten clients with a method like this. I think what you have in AdminClientImpl is what AdminClient should look like, and PublisherClient that of RoutingPublisher. SubscriberClient is also discussed in #47.

[dpcollins-google]

decorates handwritten clients with a method like this

I'm not doing that. It makes debugging this code so confusing (source: I've tried to debug issues with the python client library in the past). Composition is much more understandable than inheritance/decoration.

I've created #54 demonstrating what the publisher client would look like, but I don't think this necessarily provides any value (its effectively a settings holder with no behavior). If this is preferable though I can make the equivalent change for the subscribe side.

[dpcollins-google]

I think what you have in AdminClientImpl is what AdminClient should look like

I don't want to mix the interface and the code. I can implement a wrapper class distinct from the interface that looks like the generated client, but I'm not going to put AdminClientImpl there.

[anguillanneuf]

Even #54 is odd because one has to call .make_publisher() on PublisherClient to make a publisher client. Any suggestions here? @googleapis/yoshi-python @busunkim96

[dpcollins-google]

Publisher needs to be a context manager to clean up the associated stream and thread resources. If there is a different pattern for resource cleanup that is preferred I can use that instead, but context managers are AFAIK the blessed way to do this in python.

[tswast]

In the BigQuery Storage API, we need to wrap the generated clients too. We subclass them here:

https://github.com/googleapis/python-bigquery-storage/blob/fbbb439b8c77fa9367a4b5bea725dd0b0f26b769/google/cloud/bigquery_storage_v1/client.py#L36

This makes sure our constructor has the same pattern as all the other Google Cloud client libraries.

[busunkim96]

Currently client objects (be they handwritten or generated) are consistently imported as something similar tofrom google.cloud.foo import BarClient. I think there is value to preserving that pattern for new libraries.

@dpcollins-google There is some proposed work to allow client objects to behave as context managers. googleapis/gapic-generator-python#575 You could possibly use a similar implementation for Publisher.

[dpcollins-google]

We subclass them here:

I do not want to subclass them. The constructor can have the same call pattern without subclassing, but if we were to subclass, we would add methods just to then remove them (by overwriting them with methods of the same name, which would have to be done for every method)

Currently client objects (be they handwritten or generated) are consistently imported as something similar tofrom google.cloud.foo import BarClient. I think there is value to preserving that pattern for new libraries.

The problem is that the client would then have to be split between sync and async to be usable as both a sync and async context manager, and managing the lifetime of multiple single-topic streams, meaning it would lose the "client fails permanently on permanent errors" aspect. If this is the way we want to go, it would make both the publisher and subscriber client more similar to CPS, and I could split it into sync and async versions, but it would add significant implementation complexity. I will provide an example of how this would look (with tests to be added later)

[dpcollins-google]

Look at #56, thoughts?

[anguillanneuf]

#56 is much closer to the user experience with Pub/Sub. I left some comments.

I'm less concerned about inefficiencies in the inheritance pattern than "client fails permanently on permanent errors". Can you describe what that means in an example? It looks like Pub/Sub uses both sync and async clients too.

[dpcollins-google]

client fails permanently on permanent errors

This is no longer the case with the multiplexed clients, which is part of why I wanted to avoid this. Essentially: when you publish or subscribe, any retryable errors are retried, and the Subscriber or Publisher objects become unusable due to the non-retryable error. This is no longer the case with #56, as it will recreate failed clients for new operations. However, those new clients are just as likely to fail, as the previous publish client (for example) would ONLY have failed on something like INVALID_ARGUMENT or FAILED_PRECONDITION. The user is no longer forced to handle this error, but can treat it as a normal singleton RPC failure and retry the future in userland. The previous construction intentionally makes this pattern difficult, as the next stream will almost certainly also fail with the same error.

[manuelmenzella-google]

This is no longer the case with the multiplexed clients, which is part of why I wanted to avoid this. Essentially: when you publish or subscribe, any retryable errors are retried, and the Subscriber or Publisher objects become unusable due to the non-retryable error. This is no longer the case with #56, as it will recreate failed clients for new operations. However, those new clients are just as likely to fail, as the previous publish client (for example) would ONLY have failed on something like INVALID_ARGUMENT or FAILED_PRECONDITION. The user is no longer forced to handle this error, but can treat it as a normal singleton RPC failure and retry the future in userland.

With #56, only non-retryable errors would surface to the client and fail the future, right? Retryable errors would be retried silently. Is this really such a big behavioral difference?