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.

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

Add reactive MS SQL client extension #18311

Merged
merged 7 commits into from
Jul 6, 2021
Merged

Add reactive MS SQL client extension #18311

merged 7 commits into from
Jul 6, 2021

Conversation

tsegismont
Copy link
Contributor

  • runtime/deployment modules
  • integration tests (including native mode)
  • datasources docs

@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure labels Jul 1, 2021
@tsegismont
Copy link
Contributor Author

cc @Sanne @DavideD

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 1, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 761fe72

Status Name Step Test failures Logs Raw logs
✖ Initial JDK 11 Build Verify extension dependencies ⚠️ Check → Logs Raw logs

@gsmet gsmet changed the title MS SQL extension Add reactive MS SQL client extension Jul 1, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 1, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building b03500c

Status Name Step Test failures Logs Raw logs
✖ JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
✖ JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs

⚠️ Errors occurred while downloading the Surefire reports. This report is incomplete.

@Sanne
Copy link
Member

Sanne commented Jul 2, 2021

@DavideD could you follow up with integration tests using MS SQL + Hibernate Reactive ?
Which I assume will need an HR release ?

@DavideD
Copy link
Contributor

DavideD commented Jul 2, 2021 •
edited
Loading

Do you mean integration tests for Quarkus? Sure, we are having some trouble with the MS SQL Client on Hibernate Reactive at the moment, but as soon as it has been fixed, I will add them

@tsegismont
Copy link
Contributor Author

Thank you all for the review. I addressed the concerns and rebased on main. This allowed to add support for the TDS packet size option that comes with Vert.x 4.1.1.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 2, 2021 •
edited
Loading

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 40baa0d

Status Name Step Test failures Logs Raw logs
✖ JVM Tests - JDK 11 Build Test failures Logs Raw logs
✖ JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/elytron-undertow

✖ io.quarkus.it.undertow.elytron.BaseAuthRestTest.testPost line 28 - More details - Source on GitHub

@Sanne
Copy link
Member

Sanne commented Jul 2, 2021

Looks like the failures are related:

2021-07-02T18:09:30.9633222Z [ERROR] Failed to execute goal io.fabric8:docker-maven-plugin:0.31.0:start (docker-start) on project quarkus-integration-test-reactive-mssql-client: I/O Error: [mcr.microsoft.com/mssql/server:2019-latest] "quarkus-test-mssql": Timeout after 10045 ms while waiting on tcp port '[/172.17.0.2:1433]' -> [Help 1]

@Sanne
Copy link
Member

Sanne commented Jul 2, 2021

it's a DB bootstrap timeout:

2021-07-02T17:29:38.5268124Z [ERROR] DOCKER> [mcr.microsoft.com/mssql/server:2019-latest] "quarkus-test-mssql": Timeout after 10045 ms while waiting on tcp port '[/172.17.0.2:1433]'

@Sanne
Copy link
Member

Sanne commented Jul 2, 2021

@tsegismont could you copy the infrastructure from the pom.xml in integration-tests/jpa-mssql ? It seems your pom.xml is similar but doesn't have crucial elements like the timeout settings.

cescoffier
cescoffier approved these changes Jul 5, 2021
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

It looks good to me. However, validation by Sanne would be great (as I don't know much about databases anyway).

@tsegismont
Copy link
Contributor Author

@tsegismont could you copy the infrastructure from the pom.xml in integration-tests/jpa-mssql ? It seems your pom.xml is similar but doesn't have crucial elements like the timeout settings.

@Sanne good idea, I will do it for both the integration tests and the deployment module tests.

@tsegismont
Update MSSQL container setup to avoid timeouts
c2d0747 
Took example from JPA-MSSQL integration tests module.
Sanne
Sanne approved these changes Jul 5, 2021
View reviewed changes
extensions/reactive-mssql-client/deployment/pom.xml
</tcp>
<log>SQL Server is now ready for client connections</log>
<!-- Unfortunately booting this is slow, needs to set a generous timeout: -->
<time>40000</time>
Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks! Let's see if CI managed to run it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one failure but it looks unrelated

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 5, 2021

Failing Jobs - Building c2d0747

Status Name Step Test failures Logs Raw logs
✖ JVM Tests - JDK 11 Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.shutdown.ShutdownTest. line 56 - More details - Source on GitHub

@Sanne
Copy link
Member

Sanne commented Jul 6, 2021

awesome, let's merge this. Many thanks!

@Sanne Sanne merged commit cdc47c3 into quarkusio:main Jul 6, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jul 6, 2021
@tsegismont tsegismont deleted the ms_sql_extension branch September 9, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sanne Sanne Sanne approved these changes

@cescoffier cescoffier cescoffier approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure release/noteworthy-feature
Projects
None yet
Milestone
2.1.0.CR1
Development

Successfully merging this pull request may close these issues.
5 participants
@tsegismont @Sanne @DavideD @cescoffier @gsmet