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

Feature/reactive panache #192

Merged
merged 1 commit into from
Dec 22, 2021
Merged

Feature/reactive panache #192

merged 1 commit into from
Dec 22, 2021

Conversation

fedinskiy
Copy link
Contributor

@fedinskiy fedinskiy commented Aug 18, 2021 •
edited
Loading

Tests for hibernate-reactive in general and Panache in particular.
Test plans for these changes can be found here: https://github.com/quarkus-qe/quarkus-test-plans/blob/main/QUARKUS-1079.md

Progress:

  • join commits into one
  • description in README
  • decide, if it's worth to create a new module instead of changing the existing sql-db-app
  • Add some OpenShift scenarios
  • Coverage for remove method
  • openSession, withSession, transaction
  • Query method: setMaxResults
  • validation of values without panache(Looks like it's not working now, will need to create a reproducer)
  • DTO projection
  • Attribute converters
  • Stateless sessions and their methods
  • Queries: named, native

@fedinskiy fedinskiy requested a review from Sgitario August 18, 2021 14:12
@fedinskiy
Copy link
Contributor Author

fedinskiy commented Aug 18, 2021 •
edited
Loading

TODO:

  • join commits into one
  • description in README
  • decide, if it's worth to create a new module instead of changing the existing sql-db-app
  • Add some OpenShift scenarios
  • Coverage for remove method
  • openSession, withSession, transaction
  • Query method: setMaxResults
  • validation of values without panache
  • DTO projection
  • Attribute converters
  • Stateless sessions and their methods
  • Queries: named, native

Sgitario
Sgitario suggested changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

Besides the comments, why not adding some OpenShift scenarios too?
Adding @pjgg as reviewer since it did the Vert.x SQL module and has experience with reactive.

@Sgitario Sgitario requested a review from pjgg August 19, 2021 06:21
pjgg
pjgg reviewed Aug 30, 2021
View reviewed changes
pjgg
pjgg reviewed Aug 30, 2021
View reviewed changes
@pjgg pjgg removed the backport/2.2 label Sep 1, 2021
@rsvoboda rsvoboda requested a review from kshpak November 8, 2021 09:04
@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch from 86bc5a6 to ed73c2b Compare November 8, 2021 16:00
@fedinskiy fedinskiy marked this pull request as ready for review November 8, 2021 16:01
@fedinskiy fedinskiy requested a review from pjgg November 9, 2021 08:31
@rsvoboda
Copy link
Member

rsvoboda commented Nov 9, 2021

@fedinskiy how long do the newly added tests run in JVM and Native mode ?

@fedinskiy
Copy link
Contributor Author

fedinskiy commented Nov 9, 2021 •
edited
Loading

@rsvoboda
Local machine, JVM:
34:26 min with DB2
02:12 min without

Local machine, Native
13:12 min without DB2

Server, JVM
04:04 min without DB2

Server, Native
22:40 min without DB2

@kshpak
Copy link
Contributor

kshpak commented Nov 9, 2021 •
edited
Loading

Review in progress ...

@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch 2 times, most recently from 3ab1f3a to 758da1c Compare November 9, 2021 15:49
kshpak
kshpak suggested changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@kshpak kshpak left a comment

Choose a reason for hiding this comment

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

This is not a complete review, but there are already enough things to change. Please respond to the questions and update the PR. I will take a look once again after the PR is updated

@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch 5 times, most recently from 68e0139 to 749a287 Compare November 10, 2021 14:34
@fedinskiy fedinskiy requested a review from kshpak November 10, 2021 14:37
@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch 3 times, most recently from 4381370 to c7e4156 Compare November 11, 2021 09:45
kshpak
kshpak reviewed Nov 11, 2021
View reviewed changes
sql-db/hibernate-reactive/src/main/java/io/quarkus/ts/reactive/Book.java Outdated
private String title;

@NotNull
private Integer author;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can map it by OneToMany <-> ManyToOne, this shouldn't be a problem

@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch 8 times, most recently from 5a95696 to f264eb2 Compare November 26, 2021 12:27
@fedinskiy fedinskiy requested a review from pjgg December 2, 2021 08:03
@pjgg
Copy link
Contributor

pjgg commented Dec 3, 2021

run tests

@fedinskiy
Copy link
Contributor Author

@pjgg OpenShift tests are now run as a part of standard checks. If you want, you can rerun them there: https://github.com/quarkus-qe/quarkus-test-suite/runs/4334102321?check_suite_focus=true

pjgg
pjgg approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@pjgg pjgg left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for your contribution!

@kshpak
Copy link
Contributor

kshpak commented Dec 6, 2021

run tests

kshpak
kshpak suggested changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@kshpak kshpak left a comment

Choose a reason for hiding this comment

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

Please resolve the last two comments && it's ready to be merged

@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch from f264eb2 to 07ee21a Compare December 6, 2021 16:44
kshpak
kshpak suggested changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@kshpak kshpak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Missed this one before. Remove bare metal test for Postgresql10DatabaseIT and place the logic in OpenShift variant of it. The same goes for Postgresql12DatabaseIT

kshpak
kshpak suggested changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@kshpak kshpak left a comment

Choose a reason for hiding this comment

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

Discussed changes

@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch 3 times, most recently from 1114b69 to 0a533c1 Compare December 8, 2021 15:24
@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch 2 times, most recently from 6eaa8cb to e48bd42 Compare December 15, 2021 15:56
@fedinskiy fedinskiy force-pushed the feature/reactive-panache branch from e48bd42 to 4b6cda1 Compare December 15, 2021 16:42
@pjgg
Copy link
Contributor

pjgg commented Dec 22, 2021

Kyrylo comments were already reflected.

@pjgg pjgg merged commit 197d40c into quarkus-qe:main Dec 22, 2021
@kshpak
Copy link
Contributor

kshpak commented Jan 3, 2022

Kyrylo comments were already reflected.

Thanks for the final merge :)

@pjgg pjgg mentioned this pull request Jan 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sgitario Sgitario Sgitario requested changes

@kshpak kshpak kshpak requested changes

@pjgg pjgg pjgg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fedinskiy @rsvoboda @kshpak @pjgg @Sgitario