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

Reproducer for leaking connection in XA mode #1919

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

fedinskiy
Copy link
Contributor

Summary

When XA is enabled, connections to MS SQL may leak. This tests checks, that this is not happening.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

roughly 4,5 minutes gone, that's sad, I'd prefer if we could avoid this test. changes lgtm, thanks

@fedinskiy fedinskiy force-pushed the reproducer/xa-leak branch from 1f81ef4 to 1b96617 Compare August 9, 2024 10:42
@fedinskiy
Copy link
Contributor Author

@michalvavrik @rsvoboda I am in doubts wrt this one. If we enable it, we lose several minutes on every run, otherwise we risk the test too turn stale and for the error to reappear.
Alternative solutions:

  1. Disable it for main, but enable in branches
  2. Enable it only when there is an environment variable/system property(which one).

What do you think? Maybe you have other suggestions?

@michalvavrik
Copy link
Member

@michalvavrik @rsvoboda I am in doubts wrt this one. If we enable it, we lose several minutes on every run, otherwise we risk the test too turn stale and for the error to reappear. Alternative solutions:

  1. Disable it for main, but enable in branches
  2. Enable it only when there is an environment variable/system property(which one).

What do you think? Maybe you have other suggestions?

I feel same way. I think it would be nice to run this test with a product Quarkus version. Problem with branches is that we will forget to put it into next branches and miss possible regression. Enable it explicitly with env var is fine with me as well, though it is something new we would have to add to Jenkins (maybe discuss this with @mjurc as well).

@mjurc
Copy link
Member

mjurc commented Aug 9, 2024

I would keep it in main too personally, this should be ran in weeklies for regression checks.

I don't mind excluding this for CI (especially considering that databases module already takes ages), but I would like to have this in main. As for method of exclusion - Maven exclusion, envvar - I don't really have a strong opinion here

@fedinskiy fedinskiy force-pushed the reproducer/xa-leak branch from 1b96617 to 49807bd Compare August 9, 2024 11:57
@fedinskiy
Copy link
Contributor Author

@mjurc do we have weeklies, that run on baremetal? Periodic build in Jenkins has only OCP.

Disabled the test on SNAPSHOTS, I suppose it is a good compromise, which doesn't require any drastic changes

@fedinskiy fedinskiy force-pushed the reproducer/xa-leak branch from 49807bd to 45b7772 Compare August 9, 2024 12:20
@mjurc
Copy link
Member

mjurc commented Aug 9, 2024

@fedinskiy Sorry, I meant the nightlies. Does this risk pushing the db modules out of allowed execution time for the runners? I don't think timeliness is that much of concern for the nightly build.

@fedinskiy
Copy link
Contributor Author

@mjurc I do not think four minutes will affect us too much.

I do not want to implement this feature in a hurry, so I created https://issues.redhat.com/browse/QQE-876 to discuss a way to do it properly.

Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

Failed test is not related to the PR

@rsvoboda
Copy link
Member

Talking to Michal and he has good point, that 999-SNAPSHOT is used all the time and waiting till the product testing is risky.

We could for example define a property in https://github.com/quarkus-qe/quarkus-test-suite/blob/main/.github/workflows/ci.yml and skip the test case run using assume from JUnit.

@rsvoboda rsvoboda self-requested a review August 12, 2024 10:09
Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

We need to make the skip relevant just for GH Actions connected with PRs.

@mjurc
Copy link
Member

mjurc commented Aug 20, 2024

I'm going to merge this, the failure is unrelated and there's two approval.

Thanks!

@mjurc mjurc merged commit 6587c08 into quarkus-qe:main Aug 20, 2024
13 of 14 checks passed
@fedinskiy fedinskiy deleted the reproducer/xa-leak branch August 21, 2024 06:32
jedla97 pushed a commit to jedla97/quarkus-test-suite that referenced this pull request Sep 3, 2024
* Reproducer for leaking transactions in XA mode

https://issues.redhat.com/browse/QUARKUS-4185

* Disabled long-running tests in PR CI

https://issues.redhat.com/browse/QQE-876
(cherry picked from commit 6587c08)
@jedla97 jedla97 added this to the 3.8 milestone Sep 3, 2024
@jedla97 jedla97 mentioned this pull request Sep 3, 2024
9 tasks
rsvoboda pushed a commit that referenced this pull request Sep 6, 2024
* Reproducer for leaking transactions in XA mode

https://issues.redhat.com/browse/QUARKUS-4185

* Disabled long-running tests in PR CI

https://issues.redhat.com/browse/QQE-876
(cherry picked from commit 6587c08)
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.

5 participants