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!: [Spanner] Upgrade to V2 #7193

Open
wants to merge 76 commits into
base: main
Choose a base branch
from
Open

feat!: [Spanner] Upgrade to V2 #7193

wants to merge 76 commits into from

Conversation

ajupazhamayil
Copy link
Contributor

@ajupazhamayil ajupazhamayil commented Apr 1, 2024

Spanner V2:

Part 1:

  • Replacing Connection object with RequestHandler [Operation class]
  • Replacing Connection object with RequestHandler [Database class]
  • Replacing Connection object with RequestHandler [Session class]
  • Replacing Connection object with RequestHandler [CacheSessionPool class]
  • Introduce LongRunningOperationsManager [Replacing Connection object with RequestHandler in Core's LongRunningOperation]

Part 2:

  • Replacing Connection object with RequestHandler [Backup class]
  • Replacing Connection object with RequestHandler [Instance class]
  • Replacing Connection object with RequestHandler [InstanceConfiguration class]
  • Replacing Connection object with RequestHandler [SpannerClient]
  • Remove Connection Classes and Directory 🎉

Tests:

  • System tests
  • Unit tests
  • Snippet tests
  • System tests
  • Unit tests
  • Snippet tests

Related work:

  • Migration.md file
  • Update Admin Clients to Use V2 OperationsClient [GAPIC generator change] [See @todo in Spanner/src/Admin/Database/V1/Client/DatabaseAdminClient.php] : This is replaced with the migration status changes.

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

@ajupazhamayil ajupazhamayil force-pushed the spanner-v2 branch 2 times, most recently from 843be5e to 55a81e9 Compare April 1, 2024 18:01
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 see this is in draft mode, but the design is confusing to me. Do you have a document or anything to describe this approach?

Core/src/LongRunning/LROTraitV2.php Outdated Show resolved Hide resolved
Core/src/LongRunning/LROTraitV2.php Outdated Show resolved Hide resolved
@ajupazhamayil
Copy link
Contributor Author

I see this is in draft mode, but the design is confusing to me. Do you have a document or anything to describe this approach?

We did not create any design document for these changes. We are following a V2 parent doc drafted by @saranshdhingra. Basically we are trying to remove usage of connection class objects and replace the usage with RequestHandler

Please let me know if you would like me to create a doc for this purpose.

@ajupazhamayil ajupazhamayil changed the title feat!: [Spanner] Upgrade to PHP V2 feat!: [Spanner] Upgrade to PHP V2 [Database + Operation] May 16, 2024
@ajupazhamayil ajupazhamayil changed the title feat!: [Spanner] Upgrade to PHP V2 [Database + Operation] feat!: [Spanner] Upgrade to PHP V2 May 16, 2024
@bshaffer bshaffer changed the title feat!: [Spanner] Upgrade to PHP V2 feat!: [Spanner] Upgrade to V2 May 23, 2024
@ajupazhamayil ajupazhamayil changed the title feat!: [Spanner] Upgrade to V2 feat!: [Spanner] Upgrade to V2 (Part 1/2) May 23, 2024
@@ -184,6 +184,9 @@ public function getOperationsClient()
public function resumeOperation($operationName, $methodName = null)
{
$options = isset($this->descriptors[$methodName]['longRunning']) ? $this->descriptors[$methodName]['longRunning'] : [];
// OperationResponse is a V1 surface class, we need V2 surface class.
// [V2 LongRunning/Client/OperationsClient, getOperation]
// Other clients have also used the OperationResponse similarly.
$operation = new OperationResponse($operationName, $this->getOperationsClient(), $options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes can be removed once the migration mode is fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should modify the MIGRATION_MODE locally and generate the client as part of this PR, so we can preview those changes. Then we can merge the CL to change the migration mode once this PR is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed internally that Brent will take care of the MIGRATION_MODE, Thanks!

@ajupazhamayil ajupazhamayil changed the title feat!: [Spanner] Upgrade to V2 (Part 1/2) feat!: [Spanner] Upgrade to V2 Jun 12, 2024
@bshaffer
Copy link
Contributor

The "Prefer Lowest" tests are failing because you've added a new Trait to google/cloud-core, you need to upgrade the minimum version of cloud core specified in Spanner/composer.json

@ajupazhamayil
Copy link
Contributor Author

ajupazhamayil commented Jun 13, 2024

The "Prefer Lowest" tests are failing because you've added a new Trait to google/cloud-core, you need to upgrade the minimum version of cloud core specified in Spanner/composer.json

Great, the test is passing now..

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.

None yet

2 participants