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

feat!: Remove the use of connection classes in PubSub. #6872

Merged
merged 78 commits into from
Feb 9, 2024

Conversation

saranshdhingra
Copy link
Contributor

@saranshdhingra saranshdhingra commented Dec 13, 2023

This PR:

  • Introduces the new V2 GAPIC surface for Cloud PubSub.
  • Uses the newly introduced RequestHandler flow in the following classes:
  • PubSubClient, Topic, Subscription, Schema and the `Snapshot.

BREAKING_CHANGE_REASON=RC-1 for the version 2 of the PubSub library.

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

I know we discussed going straight to V2, so that we wouldn't have to maintain two codepaths, and to be extra careful as to not break our customers. Is that still the path you'd like to take? I think this is our best option, but we'll want to mark things deprecated ASAP in V1 (such as the gapicClient options) so that we can give customers some time (generally, 3 months) before removing them in V2.

As far as I know, the "breaking changes" we're planning are for options and classes that should have been marked internal in the first place, so the "3 months" may be flexible. However, the more time we give our customers the better!

PubSub/src/PubSubClient.php Outdated Show resolved Hide resolved
PubSub/src/PubSubClient.php Show resolved Hide resolved
PubSub/src/PubSubClient.php Outdated Show resolved Hide resolved
PubSub/src/Schema.php Show resolved Hide resolved
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

A main concern here is that all these calls should be using the PubSub V2 GAPIC clients (e.g. Google\Cloud\PubSub\V1\Client\SubscriberClient)

PubSub/src/Subscription.php Outdated Show resolved Hide resolved
PubSub/src/Subscription.php Outdated Show resolved Hide resolved
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

One important thing we still need to do here is remove all V1 GAPIC classes and all deprecated backwards compatibility files. That includes:

V1 GAPIC files

  • src/V1/PublisherClient.php
  • src/V1/SchemaServiceClient.php
  • src/V1/SubscriberClient.php
  • src/V1/PublisherGrpcClient.php
  • src/V1/SchemaServiceGrpcClient.php
  • src/V1/SubscriberGrpcClient.php
  • src/V1/Gapic/PublisherGapicClient.php
  • src/V1/Gapic/SchemaServiceGapicClient.php
  • src/V1/Gapic/SubscriberGapicClient.php
  • tests/Unit/V1/PublisherClientTest.php
  • tests/Unit/V1/SchemaServiceClientTest.php
  • tests/Unit/V1/SubscriberClientTest.php

Backwards Compatibility Files

  • src/V1/BigQueryConfig_State.php
  • src/V1/CloudStorageConfig_AvroConfig.php
  • src/V1/CloudStorageConfig_State.php
  • src/V1/CloudStorageConfig_TextConfig.php
  • src/V1/PushConfig_OidcToken.php
  • src/V1/Schema_Type.php
  • src/V1/StreamingPullResponse_AcknowledgeConfirmation.php
  • src/V1/StreamingPullResponse_ModifyAckDeadlineConfirmation.php
  • src/V1/StreamingPullResponse_SubscriptionProperties.php
  • src/V1/Subscription_State.php

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Review of migrating.md. Mostly good with a few important changes.

PubSub/migrating.md Outdated Show resolved Hide resolved
PubSub/migrating.md Outdated Show resolved Hide resolved
PubSub/migrating.md Outdated Show resolved Hide resolved
PubSub/migrating.md Outdated Show resolved Hide resolved
PubSub/migrating.md Outdated Show resolved Hide resolved
Base automatically changed from add-request-handler to main January 26, 2024 18:38
@bshaffer
Copy link
Contributor

I know we went over this, but this code throws an error in the new version (but doesn't in the old version):

$pubsub = new PubSubClient();
$topics = $pubsub->topics();

The error is:

PHP Fatal error:  Uncaught Google\Cloud\Core\Exception\GoogleException: No project ID was provided, and we were unable to detect a default project ID. in /Users/betterbrent/Code/tpc/vendor/google/cloud/Core/src/ClientTrait.php:231

PubSub/composer.json Outdated Show resolved Hide resolved
@bshaffer bshaffer enabled auto-merge (squash) February 9, 2024 18:45
@bshaffer bshaffer merged commit 70e2265 into main Feb 9, 2024
28 checks passed
@bshaffer bshaffer deleted the add-request-handler-pubsub branch February 9, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants