Skip to content

Set MySqlClientBatchingBatcher as a default batcher for MySqlDataDriver #1600

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 10, 2018
Merged

Set MySqlClientBatchingBatcher as a default batcher for MySqlDataDriver #1600

merged 3 commits into from
Mar 10, 2018

Conversation

fredericDelaporte
Copy link
Member

@fredericDelaporte fredericDelaporte commented Mar 8, 2018 •
edited
Loading

Setting MySql batcher as the default batcher seems to have been forgotten when it was added. (At least no one answered my question about this on its ticket.)

Also fix #1604 (no translation of data provider exception) and fix #1605 (may not close pending data reader before initiating a batch).

@fredericDelaporte fredericDelaporte added this to the 5.1 milestone Mar 8, 2018
hazzik
hazzik reviewed Mar 8, 2018
View reviewed changes
src/NHibernate/Driver/MySqlDataDriver.cs
@@ -95,5 +96,7 @@ public override bool SupportsMultipleQueries
// https://dev.mysql.com/doc/refman/5.7/en/datetime.html
/// <inheritdoc />
public override DateTime MinDate => new DateTime(1000, 1, 1);

System.Type IEmbeddedBatcherFactoryProvider.BatcherFactoryClass => typeof(MySqlClientBatchingBatcherFactory);
Copy link
Member

Choose a reason for hiding this comment

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

Shall this be guarded by #if NETFX?

Copy link
Member

Choose a reason for hiding this comment

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

Seems not. It's ok then.

hazzik
hazzik previously approved these changes Mar 8, 2018
View reviewed changes
@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 9, 2018 •
edited
Loading

It seems the MySql batcher causes issues not under .Net Core but under .Net Framework!

Update: Six of them are anecdotal: the thrown error through the batch has not the same shape. Well, it may still be a breaking change for those having configured a batch-size while not having actually enabled batching, letting MySql using the non batching batcher. Since this is somewhat an erroneous configuration, I think such a change is a grey area change and should be accepted in a minor.

The six others are more problematic. They look like the errors we ignore in .Net Core builds. And we have also more errors in those builds now.

@fredericDelaporte fredericDelaporte changed the title Provide MySql driver default batcher. [WIP] Provide MySql driver default batcher. Mar 9, 2018
@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 9, 2018 •
edited
Loading

The MySql batcher is encapsulating all exceptions occurring in ExecuteNonQuery into a HibernateException. This prevents the exception translation mechanism to take place, which is a bug.

I am fixing it inside this PR because the bug is showcased by current tests only if the batcher is defined as the defaut for MySql. For keeping track of this bug, I have added #1604.

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 9, 2018 •
edited
Loading

The six others are more problematic. They look like the errors we ignore in .Net Core builds. And we have also more errors in those builds now.

The MySql batcher misses a CheckReaders call causing it to try initiating a new batch while a reader is still open on the connection, which MySql does not support. This is another bug of the batcher: #1605. I am willing to fix it likewise in this PR, which is now a finalization of NH-2778 which has introduced this batcher.

@fredericDelaporte fredericDelaporte changed the title [WIP] Provide MySql driver default batcher. [WIP] Provide MySql driver default batcher and fix it. Mar 9, 2018
@fredericDelaporte fredericDelaporte changed the title [WIP] Provide MySql driver default batcher and fix it. Finalize MySql batcher Mar 9, 2018
@fredericDelaporte
Copy link
Member Author

This PR is back to 4 failures for the AppVeyor MySql build and 8 for the Travis one.

@hazzik hazzik changed the title Finalize MySql batcher Set MySqlClientBatchingBatcher as a default batcher for MySqlDataDriver Mar 10, 2018
@hazzik hazzik merged commit 9514cd2 into nhibernate:master Mar 10, 2018
@fredericDelaporte fredericDelaporte deleted the MySqlBatcher branch March 10, 2018 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hazzik hazzik hazzik left review comments

Assignees
No one assigned
Labels
c: DataProviders / Dialects p: Minor r: Fixed t: Improvement
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

MySql batcher may attempt initiating a new batch without closing open reader first. MySql batcher disables db exception translation
2 participants
@fredericDelaporte @hazzik