Skip to content

feat: bump dependencies to ef9#561

Open
abbottdev wants to merge 10 commits intogoogleapis:mainfrom
abbottdev:feature/ef-9
Open

feat: bump dependencies to ef9#561
abbottdev wants to merge 10 commits intogoogleapis:mainfrom
abbottdev:feature/ef-9

Conversation

@abbottdev
Copy link
Contributor

This goes some way to solving #481 - be the change you want to see 😄

If we could start the ball rolling on this - that'd be great. The unit tests are currently passing, but I've not validated the Integration tests at this stage.

Needs more documentation updates potentially - as well as the nice to haves such as adding support for the new EF 9.0 features - but I think they could be follow up PRs.

  • Tests pass
  • Appropriate changes to README are included in PR

@google-cla
Copy link

google-cla bot commented Jun 9, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@abbottdev
Copy link
Contributor Author

Just bumped the SDK versions in the workflow definitions. (CLA agreement is in progress my side - doing our org-level sign offs offline.)

@abbottdev
Copy link
Contributor Author

abbottdev commented Jun 10, 2025

UPDATE:
Test summary: total: 136, failed: 4, succeeded: 131, skipped: 1, duration: 4.7s

Failures are 3 table splitting tests with Schema change operation rejected because a concurrent schema change operation or read-write transaction is already in progress

And the CanSeedData test is getting a mismatch count.

@abbottdev
Copy link
Contributor Author

abbottdev commented Jun 10, 2025

OK, so I'm at a point where I could do with a steer. There's only 1 remaining failing unit test. All integration tests are passing locally.

The changes I've made in SpannerMigrationCommandExecutor have brought it's behaviour inline with the default MigrationCommandExecutor in EF Core.

The trouble I have is that the Spanner implementation was previously batching the DML + DDL requests and using their batch methods on the underlying spanner connection. I had to do this because it wasn't respecting the ambient transaction scope from EF Migration - so it would fail the integration tests.

The Test assertion is failing because the Assertion is expecting a batch, but consequentially it's no longer a batch.

In the MigrationExecutionState there is a LastCommittedCommandIndex property (this is managed by EF Core). It feels wrong to go against the grain by batching as that would make that LastCommittedCommandIndex pointless - but at the same time - it makes just as much sense to use the Batch DML/DDL methods.

After a steer - do I make the tests pass as is, and don't use the batch DML/DDL - or - ignore the EF LastCommittedCommandIndex property and batch it anyway?

(sorry for the actions spam @olavloite)

@olavloite
Copy link
Collaborator

@abbottdev

In the MigrationExecutionState there is a LastCommittedCommandIndex property (this is managed by EF Core). It feels wrong to go against the grain by batching as that would make that LastCommittedCommandIndex pointless - but at the same time - it makes just as much sense to use the Batch DML/DDL methods.

I'm not familiar with what changes in EF9 causes this to be different, but in general: Batching both DML and DDL is very important for Spanner. The execution speed of a DDL batch consisting of for example 10 statements, is comparable to that of a single DDL statement (assuming that the batch does not contain any statements that need backfilling or constraint validation of existing data, like adding a new index to an existing table). Executing 10 DDL statements serially is much slower, and risks getting throttled by Spanner, both due to the limit on the number of administrative requests per second, and due to the fact that Spanner allows you to query each version of the database schema. The latter puts a limit on the number of schema changes that are allowed per unit of time.

So disabling batching is most probably not something that we would want.

On a different note: This provider is normally only updated to the LTS versions of Entity Framework. EF9 is not one of those. In addition, updating the EF version that is used by the provider is a breaking change, meaning that we would need a major version bump. So there will be some additional work needed here before we could potentially merge this.

@abbottdev
Copy link
Contributor Author

@olavloite I see where you're coming from. But from my - and I believe many others, perspective in the .NET ecosystem - we will be targeting the current release - which in this case is EF9. https://learn.microsoft.com/en-us/ef/core/what-is-new/

Perhaps this should be a preview/beta release so that we can start to get feedback and prepare for the transition. Would having a preview/beta version a happy middleground? Equally I could start a EF9 fork but I don't feel that's probably the most helpful way to contribute.

If a preview/beta is something we'd be happier with, what work can I do to help enable a preview/beta version of this provider? 😄

In the mean time - I'll work on getting a working batched DML/DDL implementation back in with the working tests 👍

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.

2 participants