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

Added support for the CancelQuery() method in IStatelessSession #3074

Merged
merged 18 commits into from
Sep 27, 2022
Merged

Added support for the CancelQuery() method in IStatelessSession #3074

merged 18 commits into from
Sep 27, 2022

Conversation

SoftStoneDevelop
Copy link
Contributor

Resolve #809

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution,

We do not accept PR without corresponding tests. See contributing. May you add a test?

Copy link
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

I have not checked all databases on my local setup, so waiting for the automated tests to do this for me.

@fredericDelaporte
Copy link
Member

On GitHub action, AFAIK we are running SQL Server under Linux, and we seem to hit dotnet/SqlClient#109. It has been fixed in Microsoft.Data.SqlClient 2.0, but we are using System.Data.SqlClient, which is unlikely to be fixed, since Microsoft has announced it will go on developing only Microsoft.Data.SqlClient.

Should we switch Sql Server tests to Microsoft.Data.SqlClient? Well, ideally I think we should go on testing System.Data.SqlClient under .Net Framework, but test Microsoft.Data.SqlClient under net6.0.

@fredericDelaporte
Copy link
Member

@hazzik, it seems this cancel test freezes Oracle test suit. (Maybe Oracle does not support Cancel...) The NHibernate agent on TeamCity seems gone. Can you check what is going on please?

@hazzik

This comment was marked as off-topic.

@hazzik
Copy link
Member

hazzik commented Jun 28, 2022

Should we switch Sql Server tests to Microsoft.Data.SqlClient?

We should at least add it to the test suit. Maybe we test it in both platforms?

Vyacheslav added 2 commits June 29, 2022 05:30
Changed on 'cancel': In oracle message contain word 'cancel'(ORA-01013: user requested cancel of current operation) and not 'cancelled' or 'canceled'. Word 'cancel' contains in message for all database types.
@hazzik

This comment was marked as off-topic.

@bahusoid

This comment was marked as off-topic.

@hazzik

This comment was marked as off-topic.

@bahusoid

This comment was marked as off-topic.

@hazzik hazzik changed the title Added support for the CancelQuery() method in IStatelessSession (#809) Added support for the CancelQuery() method in IStatelessSession Jul 20, 2022
hazzik
hazzik previously approved these changes Jul 20, 2022
@hazzik

This comment was marked as off-topic.

@hazzik
Copy link
Member

hazzik commented Jul 20, 2022

Ok, agent is back. I've also increased tier for the VM, so builds are slightly faster now.

@hazzik
Copy link
Member

hazzik commented Jul 21, 2022

For some reason I cannot get any logs for Oracle

@hazzik
Copy link
Member

hazzik commented Jul 21, 2022

Oracle on TC works fine :-/

@SoftStoneDevelop
Copy link
Contributor Author

@hazzik
I checked locally on Oracle 19c: test passed. Perhaps the problem is that on TC Oracle 18c. In release 18c, "Manual completion of unmanaged queries" was added (https://docs.oracle.com/en/database/oracle/oracle-database/18/newft/new-features.html#GUID-EFC832BE-DC2C-4997-8C38-8A9E27A4FFC4). But from what version 18.x?

I'll try installing 18.3 on my machine and run the test: if it passes then I have no idea why TC failed.

@SoftStoneDevelop
Copy link
Contributor Author

SoftStoneDevelop commented Aug 3, 2022

On 18.3 (on my local server) it works fine, all tests passed. The only difference from TC is that I have Oracle on Windows.

@SoftStoneDevelop
Copy link
Contributor Author

On 18.3 (on my local server) it works fine, all tests passed. The only difference from TC is that I have Oracle on Windows.

"Canceled in 360m - Oracle" misled me, I checked locally for nothing. At the TC: all test(NHibernate Oracle Managed 32) pass.

@bahusoid
Copy link
Member

"Canceled in 360m - Oracle" misled me

Why misled? There is clearly some issue with Git Hub Action build (Oracle on Linux) - https://github.com/nhibernate/nhibernate-core/runs/7643016174?check_suite_focus=true

@SoftStoneDevelop
Copy link
Contributor Author

On "Git Hub Action" yes but not on teamcity(I initially thought that the assembly hung on the TC due to deadlock). So "misled me" means that the problem is not where I thought it was.

@hazzik
Copy link
Member

hazzik commented Aug 11, 2022

But I wrote that:

Oracle on TC works fine :-/

@SoftStoneDevelop
Copy link
Contributor Author

Yes, yes I am blind, sometimes.

@bahusoid
Copy link
Member

I think we should simply skip tests for Oracle and Sql Server on linux.

@fredericDelaporte fredericDelaporte added this to the 5.4 milestone Sep 25, 2022
Copy link
Member

@bahusoid bahusoid left a comment

Choose a reason for hiding this comment

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

SupportsCancelQuery introduction looks not necessary to me (I expected skip right inside test case). But why not.

@fredericDelaporte
Copy link
Member

It makes for a central point for checking database limitations, which may help when we wish to check if some of these limitations do no more apply. Tests skipped directly inside them for some databases are harder to spot.

@hazzik

This comment was marked as resolved.

@hazzik
Copy link
Member

hazzik commented Sep 26, 2022

Created issue to track SqlServer and Oracle #3167

@fredericDelaporte fredericDelaporte merged commit 556a4a7 into nhibernate:master Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NH-2799 - Provide the CancelQuery() method in IStatelessSession
4 participants