OptaPlanner extension #7049

ge0ffrey · 2020-02-06T21:25:11Z

Works on GraalVM with -Dnative (see integration test).

ge0ffrey · 2020-02-06T21:28:29Z

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

+        }
+        if (annotationInstances.isEmpty()) {
+            throw new IllegalStateException("No classes (" + convertAnnotationInstancesToString(annotationInstances)
+                    + ") found with a @" + PlanningSolution.class.getSimpleName() + " annotation.");


We talked about removing this fail-fast in the previous PR, to follow hibernate-extension's approach, so I 'll do that in a next PR if that's ok, because of the open debate below.

Open question, what is the behavior we want:
A) When there is no @PlanningSolution class, fail fast (current implementation, differs from hibernate extension)
B) When there is no @PlanningSolution class, only fail fast if there are any quarkus.optaplanner.* application.properties, otherwise do nothing
C) When there is no @PlanningSolution class, even if there are any quarkus.optaplanner.* application.properties, always do nothing quietly

Does de hibernate extension do B) or C)?

I presume A) is a PITA when doing demo's where you grab something from code.quarkus.io and it doesn't "just run"?

Yeah. I would avoid A). I would do C with a warning instead of staying quiet if some configuration properties are defined.

gsmet

Not a real review but a reminder that we need this to be properly squashed.

I think you can already squash most if not all of the commits prior to review.

geoand · 2020-02-07T14:49:00Z

One thing I see is that the module hasen't been added to ci-templates/stages.yml.

ge0ffrey · 2020-02-07T15:15:16Z

@geoand In stages.yml, can I just add it under "kogito" on line 380? Or should it have it's own template? How do I verify that my changes didn't break anything?

geoand · 2020-02-07T15:16:57Z

Yeah, just add it under kogito and it should be run by CI automatically :)

You'll be able to verify from the name of the job - it will include your module

ge0ffrey · 2020-02-07T15:18:11Z

@geoand By the way, I got the entire thing running in GraalVM without doing special tricks in the example for OptaPlanner. No custom solution cloner, etc :) It truly just works on GraalVM.
After this, we'll work on gizmo in the upcoming months to avoid reflection entirely. That's when we'll see an improvement in the thing that every OptaPlanner user is craving for: more CPU performance.

geoand · 2020-02-07T15:18:46Z

Excellent!

ge0ffrey · 2020-02-07T16:19:23Z

@gsmet @geoand Squashed, looking forward to the review (and merge hopefully). Thanks for all your support so far!

geoand · 2020-02-07T16:29:34Z

Thanks for the hard work @ge0ffrey!

We'll hopefully be able to review some time next week.

ge0ffrey · 2020-02-11T11:13:06Z

Once this is merged, we'll work on the quickstart example that we'll build from scratch in a video about AI on quarkus. And we'll work on the guide of course :)

geoand · 2020-02-12T13:08:43Z

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

+            solverConfig = new SolverConfig(classLoader);
+        }
+        // The deployment classLoader must not escape to runtime
+        solverConfig.setClassLoader(null);


Just out of curiosity, what CL will be used at runtime as a result of this?

Most of the time, it's not used, except if a scoreDRL is active.
If it's null in that case, ScoreDirectorFactoryConfig does this at runtime, during SolverFactory.buildSolver():

ClassLoader actualClassLoader = (classLoader != null) ? classLoader : getClass().getClassLoader();

Once we start doing more work during the static init, after we internalize the SolverConfig in the SolverFactory (currently impossible due to deprecated methods that will dissappear in 8.0), I 'll have to come back to this, as that code might become a problem then.
Good question indeed. :)

What am I sort of worried about is the dev-mode. There are certain situations where getClass().getClassLoader() might not give you what you expect.
Perhaps also adding QuarkusDevModeTest is a good idea to make sure things work properly?

I 've been trying to add in QuarkusDevModeTest, but I reluctant to add a test that doesn't work in IntelliJ (it hurts productivity). In IntelliJ I get the "Undeclared build item class io.quarkus.deployment.builditem.ApplicationClassNameBuildItem" exception, but on the command line, I don't.

IntelliJ can mess up the classpath when running tests inside the Quarkus repo. I don't think it's something we need to worry about

Would it be possible to change that logic to use the TCCL if null?

The dev mode test should prove that things are OK with this change.
To be 100% Quarkus compliant, upstream here should provide a flag to use the TCCL (of the time of execution) when the classloader is null, but I don't think we need it now

Using ThreadContextClassLoader is what it does by default if we don't set it to null.
When I remove the solverConfig.setClassLoader(null); line, I get

Caused by: java.lang.RuntimeException: Unable to serialize objects of type class io.quarkus.bootstrap.classloading.QuarkusClassLoader to bytecode as it has no default constructor at io.quarkus.deployment.recording.BytecodeRecorderImpl$53.createValue(BytecodeRecorderImpl.java:1273)

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

geoand · 2020-02-12T13:29:58Z

extensions/optaplanner/runtime/pom.xml

+        <dependency>
+            <groupId>io.quarkus</groupId>
+            <artifactId>quarkus-jackson</artifactId>
+            <optional>true</optional>


Hm, have you tested to make sure this behaves the way you expect it to? And are you sure that OptaPlannerObjectMapperCustomizer is never loaded it the user doesn't add quarkus-jackson.

@gsmet WDYT about this pattern? I know it's not the way we have been doing things so far, but I do remember @ge0ffrey had a UX issue in mind when doing this, but I don't remember what it was :)

I'll check if that customizer is loaded or not. It shouldn't be, but I indeed need to verify.

I am doing it this way, because:

Having an extra extension quarkus-optaplanner-jackson-extension (and jaxb, jpa, etc), means users must explicitly add it. That's another hoop to jump through. If they forget to do that, the json marshalling of Score etc defaults to reflection way, which is hard to detect for new users.

optaplanner-spring-boot-starter does it this way, it's automagic configuration. No hoop to jump through.

If you guys insist, we can switch to the normal pattern - or leave out the jackson integration entirely for now into a separate PR so the rest can be merged already.

Well, they would have to add the optaplanner Jackson dependency anyway, no? That's not very different from adding an extension.

I wouldn't make it a blocker to merge this PR if we are sure everything works when Jackson is not there.

Fixed by extracting new quarkus-optaplanner-jackson module

...aplanner/runtime/src/main/java/io/quarkus/optaplanner/OptaPlannerObjectMapperCustomizer.java

integration-tests/optaplanner/pom.xml

gsmet

I added some comments inline. I don't think there are a lot of blockers and we could probably merge a first step and then iterate.

The CL question and the Jackson exception should probably been sorted out before merging though.

gsmet · 2020-02-14T08:48:41Z

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

+            solverConfig = new SolverConfig(classLoader);
+        }
+        // The deployment classLoader must not escape to runtime
+        solverConfig.setClassLoader(null);


Would it be possible to change that logic to use the TCCL if null?

gsmet · 2020-02-14T08:51:05Z

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

+        }
+        if (annotationInstances.isEmpty()) {
+            throw new IllegalStateException("No classes (" + convertAnnotationInstancesToString(annotationInstances)
+                    + ") found with a @" + PlanningSolution.class.getSimpleName() + " annotation.");


Yeah. I would avoid A). I would do C with a warning instead of staying quiet if some configuration properties are defined.

gsmet · 2020-02-14T08:53:02Z

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

+            // Fail fast during build to avoid a certain runtime failure
+            throw new IllegalStateException(
+                    "When using both Jackson and OptaPlanner,"
+                            + " add a dependency on org.optaplanner:optaplanner-persistence-jackson too.",


What annoys me a bit here is that if you use Jackson for your REST services and OptaPlanner without Jackson persistence, you end up with this exception. Or did I misunderstand?

Yea - the idea is that it would stick out like a soar thumb, fail-fast basically. While if you forgot to add quarkus-optaplanner-jackson, there is no fail fast: it's just hard to notice that score instances aren't being jacksoned correctly.
Anyway, not worth the hassle - I am creating quarkus-optaplanner-jackson to align with the quarkus ecosystem spirit :)

gsmet · 2020-02-14T08:53:44Z

...ner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerQuarkusConfig.java

+
+// Not named OptaPlannerConfig because classes ending with just "Config" collide with OptaPlanner's API
+@ConfigRoot(name = "optaplanner")
+public class OptaPlannerQuarkusConfig {


Call it OptaPlannerBuildTimeConfig and get rid of the comment.

Done, good point.

gsmet · 2020-02-14T08:54:31Z

...r/deployment/src/main/java/io/quarkus/optaplanner/deployment/SolverManagerQuarkusConfig.java

+     * Other options include a number or formula based on the available processor count.
+     */
+    @ConfigItem
+    Optional<String> parallelSolverCount;


That looks like a runtime property to me.

It only affects SolverManager during build time - those get injected, but we don't need to read this property at runtime.

Discussed with Georgios. He agrees, these are build time properties (they get translated into optaplanner configuration during the build time, they aren't used at runtime). Renamed class to end with BuildTimeConfig instead of QuarkusConfig to signal that.

Well, they might be build time for your code but I think it's typically values that you will want to change at runtime because it might depend on the hardware on which you deploy it.

I don't have a problem merging that as is for now but I think it's probably something we should think about for a later version (but it might be totally impractical, I don't know OptaPlanner at all).

Agreed - or users wanting to override them with -D system properties?

gsmet · 2020-02-14T08:57:55Z

extensions/optaplanner/runtime/pom.xml

+        <dependency>
+            <groupId>io.quarkus</groupId>
+            <artifactId>quarkus-jackson</artifactId>
+            <optional>true</optional>


Well, they would have to add the optaplanner Jackson dependency anyway, no? That's not very different from adding an extension.

I wouldn't make it a blocker to merge this PR if we are sure everything works when Jackson is not there.

extensions/optaplanner/runtime/pom.xml

extensions/optaplanner/runtime/src/main/resources/META-INF/quarkus-extension.yaml

...ts/optaplanner/src/main/java/io/quarkus/it/optaplanner/domain/ITestdataPlanningSolution.java

...nner/src/main/java/io/quarkus/it/optaplanner/solver/ITestdataPlanningConstraintProvider.java

ge0ffrey · 2020-02-14T09:56:24Z

@gsmet I've separated the jackson stuff into a new module called quarkus-optaplanner-jackson. (one day we'll also add quarkus-optaplanner-jaxb)

ge0ffrey · 2020-02-14T11:49:13Z

I think all review comments are handled (many adjustments done, accepted all majors and few minors, rejected a few minors), except for the fail-fast "use plan C" issue, which is for a later PR.

Let me know if there's anything left blocking the merge on this PR :)

gastaldi · 2020-02-15T12:44:12Z

Can you please squash your commits into a single one? Also it seems you need to rebase since there are conflicts from recent changes to master

ge0ffrey · 2020-02-17T10:34:10Z

@gastaldi @gsmet @geoand Rebased and squashed :)

geoand · 2020-02-17T14:35:59Z

The CI logs from the latest run are here.

The issue appears to have anything to do with this PR:

2020-02-17T13:00:43.0485880Z [INFO] -------------------------------------------------------
2020-02-17T13:00:43.0488837Z [INFO]  T E S T S
2020-02-17T13:00:43.0494363Z [INFO] -------------------------------------------------------
2020-02-17T13:00:44.3205589Z [INFO] Running io.quarkus.it.keycloak.CodeFlowTest
2020-02-17T13:00:55.8238287Z 2020-02-17 13:00:55,820 INFO  [io.quarkus] (main) Quarkus 999-SNAPSHOT started in 4.156s. Listening on: http://0.0.0.0:8081
2020-02-17T13:00:55.8242324Z 2020-02-17 13:00:55,823 INFO  [io.quarkus] (main) Profile test activated. 
2020-02-17T13:00:55.8246599Z 2020-02-17 13:00:55,824 INFO  [io.quarkus] (main) Installed features: [cdi, oidc, resteasy, resteasy-jackson, security]
2020-02-17T13:01:17.8245822Z [ERROR] Tests run: 8, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 33.494 s <<< FAILURE! - in io.quarkus.it.keycloak.CodeFlowTest
2020-02-17T13:01:17.8247068Z [ERROR] testAccessTokenInjection  Time elapsed: 1.485 s  <<< ERROR!
2020-02-17T13:01:17.8288543Z com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: Too much redirect for http://localhost:8081/web-app?state=3196cb35-59a8-4c46-9acf-f41e0ae6fd4c&session_state=09b2a60c-aa2e-452e-91c0-554d4aa9adc9&code=1d187eb5-cce0-41af-a2f1-904f2b04492c.09b2a60c-aa2e-452e-91c0-554d4aa9adc9.66d410fa-16a7-4bf6-a957-4cf37a3e0911

So I'll restart CI

geoand · 2020-02-18T09:01:45Z

CI error is:

2020-02-18T08:54:30.6540679Z [ERROR] [ERROR] Could not find the selected project in the reactor: integration-tests/optaplanner @ 
2020-02-18T08:54:30.6577421Z [ERROR] Could not find the selected project in the reactor: integration-tests/optaplanner -> [Help 1]

Seems like a problem with the PR

gsmet

Raaah, forgot to validate my review.

bom/runtime/pom.xml

extensions/optaplanner-jackson/runtime/pom.xml

extensions/optaplanner-jackson/runtime/src/main/resources/quarkus-extension.yaml

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java

gsmet · 2020-02-17T14:45:56Z

.../deployment/src/test/java/io/quarkus/optaplanner/rest/OptaPlannerProcessorHotReloadTest.java

+        test.modifyResourceFile("solverConfig.xml", s -> s.replace("<secondsSpentLimit>2</secondsSpentLimit>",
+                "<secondsSpentLimit>9</secondsSpentLimit>"));
+        resp = RestAssured.get("/solver-config/seconds-spent-limit").asString();
+        Assertions.assertEquals("secondsSpentLimit=9", resp);


I'm curious how this works without a LiveReloadBuildItem? I think we should probably have one to restart all the app in case of a change, similar to what we do elsewhere.

A HotDeploymentWatchedFileBuildItem has been added to the PR that enables the config file for reload.

extensions/optaplanner/runtime/src/main/resources/META-INF/quarkus-extension.yaml

ci-templates/stages.yml

gsmet · 2020-02-18T09:38:32Z

@geoand yes it is, thanks for your message because, apparently, I forgot to post my last review from yesterday. The name of the module is incorrect in the pipeline file.

gsmet · 2020-02-18T09:56:07Z

@ge0ffrey I think we're really not far from having this in a mergeable state. Let's try to get this in by Friday!

ge0ffrey · 2020-02-18T18:43:56Z

@gsmet Thanks for the review. All comments applied except the LiveReloadBuildItem one, because we use HotDeploymentWatchedFileBuildItem (see Georgios's comment) and we have a hot deploy test that works.

Ready for another review. Let me know if there's anything else that needs to change.

gsmet · 2020-02-19T10:26:06Z

Merged, thanks a lot for this effort. Can you work on preparing the guide and the quickstart? Thanks!

ge0ffrey · 2020-02-19T10:35:20Z

Coming soon :)

lordofthejars · 2020-03-06T18:49:52Z

@ge0ffrey @gsmet I am missing an adoc guide so everyone is aware that the support of OptaPlanner is there.

machi1990 · 2020-03-07T11:24:08Z

@ge0ffrey @gsmet I am missing an adoc guide so everyone is aware that the support of OptaPlanner is there.

Hi @lordofthejars there is a PR here #7579 to add the guide.

boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file labels Feb 6, 2020

ge0ffrey commented Feb 6, 2020

View reviewed changes

gsmet requested changes Feb 6, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

ge0ffrey requested a review from geoand February 7, 2020 09:17

ge0ffrey force-pushed the optaplanner branch from cc50a5e to 987bdf5 Compare February 7, 2020 16:16

geoand reviewed Feb 12, 2020

View reviewed changes

...planner/deployment/src/main/java/io/quarkus/optaplanner/deployment/OptaPlannerProcessor.java Outdated Show resolved Hide resolved

geoand reviewed Feb 12, 2020

View reviewed changes

...aplanner/runtime/src/main/java/io/quarkus/optaplanner/OptaPlannerObjectMapperCustomizer.java Outdated Show resolved Hide resolved

geoand reviewed Feb 12, 2020

View reviewed changes

integration-tests/optaplanner/pom.xml Outdated Show resolved Hide resolved

geoand mentioned this pull request Feb 13, 2020

WIP: OptaPlanner extension #5622

Closed

gsmet requested changes Feb 14, 2020

View reviewed changes

ge0ffrey force-pushed the optaplanner branch from 12d131b to 1c8edb3 Compare February 14, 2020 10:04

gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 15, 2020

ge0ffrey force-pushed the optaplanner branch from 06a20a2 to a9d2fdb Compare February 17, 2020 10:33

gsmet requested changes Feb 18, 2020

View reviewed changes

ge0ffrey force-pushed the optaplanner branch from a9d2fdb to 48f23da Compare February 18, 2020 18:41

ge0ffrey force-pushed the optaplanner branch from 48f23da to c3c0e69 Compare February 18, 2020 19:02

OptaPlanner extension

bc14ce6

gsmet force-pushed the optaplanner branch from c3c0e69 to bc14ce6 Compare February 19, 2020 10:25

gsmet approved these changes Feb 19, 2020

View reviewed changes

gsmet merged commit 384ed5b into quarkusio:master Feb 19, 2020

gsmet added the release/noteworthy-feature label Feb 28, 2020

gsmet added this to the 1.3.0 milestone Feb 28, 2020

OptaPlanner extension #7049

OptaPlanner extension #7049

Conversation

ge0ffrey commented Feb 6, 2020 • edited

ge0ffrey Feb 6, 2020 • edited

Choose a reason for hiding this comment

ge0ffrey Feb 7, 2020 • edited

Choose a reason for hiding this comment

ge0ffrey Feb 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

This comment has been minimized.

geoand commented Feb 7, 2020 • edited

ge0ffrey commented Feb 7, 2020

geoand commented Feb 7, 2020 • edited

ge0ffrey commented Feb 7, 2020 • edited

geoand commented Feb 7, 2020

ge0ffrey commented Feb 7, 2020

geoand commented Feb 7, 2020

ge0ffrey commented Feb 11, 2020

Choose a reason for hiding this comment

ge0ffrey Feb 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand Feb 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey Feb 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey Feb 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ge0ffrey commented Feb 14, 2020 • edited

ge0ffrey commented Feb 14, 2020

gastaldi commented Feb 15, 2020

ge0ffrey commented Feb 17, 2020

geoand commented Feb 17, 2020

geoand commented Feb 18, 2020

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Feb 18, 2020

gsmet commented Feb 18, 2020

ge0ffrey commented Feb 18, 2020 • edited

gsmet commented Feb 19, 2020

ge0ffrey commented Feb 19, 2020

lordofthejars commented Mar 6, 2020

machi1990 commented Mar 7, 2020

ge0ffrey commented Feb 6, 2020 •

edited

ge0ffrey Feb 6, 2020 •

edited

ge0ffrey Feb 7, 2020 •

edited

ge0ffrey Feb 7, 2020 •

edited

geoand commented Feb 7, 2020 •

edited

geoand commented Feb 7, 2020 •

edited

ge0ffrey commented Feb 7, 2020 •

edited

ge0ffrey Feb 12, 2020 •

edited

geoand Feb 14, 2020 •

edited

ge0ffrey Feb 12, 2020 •

edited

ge0ffrey Feb 14, 2020 •

edited

ge0ffrey commented Feb 14, 2020 •

edited

ge0ffrey commented Feb 18, 2020 •

edited