quick-perf / quickperf

QuickPerf is a testing library for Java to quickly evaluate and improve some performance-related properties
https://github.com/quick-perf/doc/wiki/QuickPerf
Apache License 2.0
488 stars 65 forks source link

Help the Micronaut Data user to fix N+1 select issues #78

Closed jeanbisutti closed 4 years ago

jeanbisutti commented 4 years ago

Description

Several SQL annotations can be used to detect N+1 select issues coming from Micronaut Data.

The aim of this issue is to display a message on console to alert of a potential N+1 select issue and to propose to use Micronaut join queries to fix it.

Implementation ideas

The way this feature is implemented for Hibernate can help you. You can look at the HibernateSuggestion class.

To test your development, execute mnv install in quickerf project and use PlayerServiceTest in the micronaut-data-jdbc module module of quickperf-examples project.

FTarfasse commented 4 years ago

Hello Jean,

I would like to work on this issue (did I ever mention that I really like the Sql annotations in Quickperf ?).

Kind regards, Fabrice

jeanbisutti commented 4 years ago

Hello Fabrice,

Great!

I assign the issue to you. Don't hesitate to tell me if I can help you.

Happy you like the SQL annotations! :)

Kind regards,

Jean

FTarfasse commented 4 years ago

Hi Jean, Here's a first version, let me know what you think. I tried so be as concise as possible.

micronaut-alert

Kind regards, Fabrice

jeanbisutti commented 4 years ago

Hi Fabrice,

Thanks a lot for this proposal.

What do you think about the following version?

Perhaps you are facing an N+1 select issue.
With Micronaut Data, you may fix it by using the @Join annotation on your repository interface: https://micronaut-projects.github.io/micronaut-data/latest/guide/#joinQueries

I think that the JPA part is not needed because the issue focuses on Micronaut Data (that would be detected from the classpath in this issue).

Kind regards,

Jean

FTarfasse commented 4 years ago

Hi Jean,

Thank you for your feedbacks ! I was unsure about the best way to speak about "micronaut-data": micronaut ? Micronaut ? Micronaut-data ? Micronaut-Data etc. Anyway, I removed the entity graph suggestion as you suggested to keep the message concise.

I also added a blank space in the JDBC suggestion message to align it (I notice it when testing, see below). "Be careful etc ..." was not aligned and it was too tempting to fix it along with the framework suggestion. Let me know if I must rollback on this modification to keep the issue 100% on MicronautSuggestion.

Here's the complete feedback from the test (the corresponding modifications are here):

micronaut-v2

P.S. : as a side note since we are speaking of quite verbose messages, the feature mentionned in issue #84 would be implemented as a maven plugin am I right (I am not familiar with developping such plugins this is why I am asking) ?

Kind regards, Fabrice

jeanbisutti commented 4 years ago

Hi Fabrice,

Many thanks! I appreciate your good quality contributions and your relevant observations.

Just one thing to add on this contribution: supply the Micronaut Data suggestion in HasSameSelectTypesWithDiffParamValuesVerifier. Apart from this, it's perfect!

I was unsure about the best way to speak about "micronaut-data": micronaut ? Micronaut ? Micronaut-data ? Micronaut-Data etc.

Micronaut is a microservice framework, and Micronaut Data is an ORM that can be used with Micronaut.

P.S. : as a side note since we are speaking of quite verbose messages, the feature mentionned in issue #84 would be implemented as a maven plugin am I right (I am not familiar with developping such plugins this is why I am asking) ?

You may refer to -D..=... that is a way to configure JVM system property from the command line.

Kind regards,

Jean

FTarfasse commented 4 years ago

Hi Jean,

Thank you for the feedbacks and clarifications. I have added the Micronaut suggestion to the missing verifier (sorry I missed that one). You can check it here. As soon as it is complete from your perspective I ll prepare a clean PR.

Regading the JVM system property (and after a bit of research), I was unaware we could create custom parameters. I'm intrigued by the way of implementing such functionality in the codebase, I think I'll have a look on this and ask you questions on the corresponding issue.

Thanks !

Kind regards, Fabrice

jeanbisutti commented 4 years ago

Hi Fabrice,

Thank you for your last modifications.

Go for the PR. :)

Regarding the JVM property, you can access a property value from System.getProperty(key). Don't hesitate if you have any questions.

Kind regards,

Jean

jeanbisutti commented 4 years ago

Fixed with 5ec7f2336b031999ac792e0244f553c7cb96622d