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

Fix Migration Batching When Seed Data Has Non-Writable Columns (I.E. RowVersion) #18320

Merged
merged 3 commits into from
Oct 16, 2019
Merged

Fix Migration Batching When Seed Data Has Non-Writable Columns (I.E. RowVersion) #18320

merged 3 commits into from
Oct 16, 2019

Conversation

crowet
Copy link
Contributor

@crowet crowet commented Oct 10, 2019

Please check if the PR fulfills these requirements

Review the guidelines for CONTRIBUTING.md for more details.

@dnfclas
Copy link

dnfclas commented Oct 10, 2019

CLA assistant check
All CLA requirements met.

@smitpatel
Copy link
Contributor

Remove all the processing around batch size. It does not fix #12466. This issue only stopped batching when the columns contained non-writable case not other cases. #12466 does not have non-writable column.

@crowet
Copy link
Contributor Author

crowet commented Oct 10, 2019

@smitpatel I removed the issue reference. This is certainly an issue, but I'm not sure if it has been reported before.

m.KeyValues,
v => Assert.Equal(45, v));
}),
builderOptions => builderOptions.UseFakeRelational(a => a.MaxBatchSize(4)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need this option? It should work without it.

Copy link
Contributor Author

@crowet crowet Oct 11, 2019

Choose a reason for hiding this comment

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

The default batch size in the Relational abstract class is 1. This fails if the batch size setting is not set. The test requires all 4 inserts included in the same batch, but when that option is removed, it becomes 4 operations.

image

Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

Please rebase this onto the release/3.1 branch and add description of the changes to the commit message

@crowet crowet changed the base branch from master to release/3.1 October 16, 2019 22:25
@AndriySvyryd AndriySvyryd merged commit afd6ca6 into dotnet:release/3.1 Oct 16, 2019
@AndriySvyryd
Copy link
Member

@crowet Thanks for your contribution!

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.

4 participants