Fixing the optaplanner guide #15702

rhuan080 · 2021-03-13T23:15:34Z

Signed-off-by: Rhuan Rocha rhuan080@gmail.com

Fixing the optaplanner guide.

gsmet · 2021-03-14T09:37:48Z

ge0ffrey

This won't compile + the guide specifically avoids fromUniquePair() as that's more complex to use

fromUniquePair() is sugar for from(A).join(A, ...)) but for beginners especially those that know SQL, it's better to use from+join.

ge0ffrey · 2021-03-15T19:11:04Z

docs/src/main/asciidoc/optaplanner.adoc

@@ -446,8 +446,7 @@ public class TimeTableConstraintProvider implements ConstraintProvider {

        // Select a lesson ...
        return constraintFactory.from(Lesson.class)
-                // ... and pair it with another lesson ...
-                .join(Lesson.class,
+                .fromUniquePair(Lesson.class,


.from(Lesson.class).fromUniquePair(...) will give a compile error

Hi @ge0ffrey, thank you for reviewing. I updated the class. Following the current guide on site, it is breaking on tests.

@rhuan080 Intresting, this does sound like an issue. Thank you for reporting and proposing a fix. I don't understand yet how these changes could fix a test yet.

Which test exactly? The TimeTableResourceTest later down the guide? Or is there a ConstraintVerifier using test that I am missing?

Hi @ge0ffrey.

Yes, I have used the TimeTableResourceTest and it fails in assertTrue(timeTable.getScore().isFeasible());. It expect the "0hard/0soft" as return, but the code is returning "-4hard/0soft". I have checked the https://github.com/quarkusio/quarkus-quickstarts/blob/main/optaplanner-quickstart/src/main/java/org/acme/optaplanner/solver/TimeTableConstraintProvider.java and I have seen it is using .fromUniquePair like me.

Hi @ge0ffrey,

I have checked this comment said by you -->fromUniquePair() is sugar for from(A).join(A, ...)) but for beginners especially those that know SQL, it's better to use from+join.

Looking at the source code, I think it has a small difference. The .fromUniquePair is filtering the planningId. It means just the planningId < PLANNING_ID_OF_FROM will match to it. I think it will generate a return with just ordered planningId. Thus I think the set of data returned from fromUniquePair() will be less than the set of data generated by from+join. Look at this code snippet:

return this.from(fromClass).join(fromClass, joiner, Joiners.lessThan(planningIdGetter));

Let me know if my thought is correct. If so, it explains the issue I have seen in the tests.

rhuan080

Hi @ge0ffrey,

I have checked this comment said by you -->fromUniquePair() is sugar for from(A).join(A, ...)) but for beginners especially those that know SQL, it's better to use from+join.

Looking at the source code, I think it has a small difference. The .fromUniquePair is filtering the planningId. It means just the planningId < PLANNING_ID_OF_FROM will match to it. I think it will generate a return with just ordered planningId. Thus I think the set of data returned from fromUniquePair() will be less than the set of data generated by from+join. Look at this code snippet:

return this.from(fromClass).join(fromClass, joiner, Joiners.lessThan(planningIdGetter));

Let me know if my thought is correct. If so, it explains the issue I have seen in the tests.

rhuan080 · 2021-03-16T19:51:54Z

docs/src/main/asciidoc/optaplanner.adoc

@@ -446,8 +446,7 @@ public class TimeTableConstraintProvider implements ConstraintProvider {

        // Select a lesson ...
        return constraintFactory.from(Lesson.class)
-                // ... and pair it with another lesson ...
-                .join(Lesson.class,
+                .fromUniquePair(Lesson.class,


Hi @ge0ffrey.

Yes, I have used the TimeTableResourceTest and it fails in assertTrue(timeTable.getScore().isFeasible());. It expect the "0hard/0soft" as return, but the code is returning "-4hard/0soft". I have checked the https://github.com/quarkusio/quarkus-quickstarts/blob/main/optaplanner-quickstart/src/main/java/org/acme/optaplanner/solver/TimeTableConstraintProvider.java and I have seen it is using .fromUniquePair like me.

ge0ffrey · 2021-03-17T07:28:56Z

Yes, I have used the TimeTableResourceTest and it fails in assertTrue(timeTable.getScore().isFeasible());. It expect the "0hard/0soft" as return, but the code is returning "-4hard/0soft".

Intresting, thank you for checking this. Yes, fromUniquePair() is indeed the addition of adding the Joiners.lessThan(planningIdGetter) as you correctly found out the actual difference.
That being said, if the use of fromUniquePair() fixes this, that's a coincidence.

How long did TimeTableResourceTest run for you when it failed?
Looking at the code, it should have run an hour:

The application.properties guarantees the solver won't stop until it reaches a feasible score (so 0 hard):

# Effectively disable this termination in favor of the best-score-limit
%test.quarkus.optaplanner.solver.termination.spent-limit=1h
%test.quarkus.optaplanner.solver.termination.best-score-limit=0hard/*soft

And the TimeTableResourceTest guarantees it will wait until the solver stops (unless the JUnit timeout is reached, which is also set to an hour, but than you'd get a different type of failure):

        while (timeTable.getSolverStatus() != SolverStatus.NOT_SOLVING) {
            // Quick polling (not a Test Thread Sleep anti-pattern)
            // Test is still fast on fast machines and doesn't randomly fail on slow machines.
            Thread.sleep(20L);
            timeTable = timeTableResource.getTimeTable();
        }

Normally, this test succeeds in a few milliseconds (small datasets) or seconds (large datasets), of course. The extremely large timings are for slow CPU's and/or use of bigger datasets.

Sources:
https://github.com/quarkusio/quarkus-quickstarts/blob/main/optaplanner-quickstart/src/main/resources/application.properties#L41
https://github.com/quarkusio/quarkus-quickstarts/blob/main/optaplanner-quickstart/src/test/java/org/acme/optaplanner/rest/TimeTableResourceTest.java#L25

rhuan080 · 2021-03-17T12:27:22Z

@ge0ffrey

I have used the properties you said and all the code provided in the guide. Based on the return from assertTrue(solution.getScore().isFeasible()), it is solving very fast with .fromUniquePair. My machine is a good machine, however, if the issue is generated by a delay from the machine, I think it better to use .fromUniquePair, as it is fast and the end-user just wants to test the OptaPlanner. It should be as fast as possible.

rhuan080 · 2021-03-19T17:08:13Z

Hi @ge0ffrey,

How long did TimeTableResourceTest run for you when it failed?
I have tested the TimeTableResourceTest running for 1 hour with from+join. It failed.

I have tested the TimeTableResourceTest running for 5s with .fromUniquePair. It worked fine.

Checking the https://github.com/quarkusio/quarkus-quickstarts/blob/main/optaplanner-quickstart, I have seen it use the same method I'm proposing here. The https://github.com/quarkusio/quarkus-quickstarts/blob/main/optaplanner-quickstart and guide are different in this point and is the point that is falling.

In my thought, it makes sense the the .fromUniquePair execute faster than from+join, as the collection generated by .fromUniquePair is less than the collection generated by from+join. Does it make sense for you?

Regarding the guide, it is just to help the end-user test it. It is better to use a code that runs in 5s all the time.

ge0ffrey · 2021-04-08T12:33:41Z

@rhuan080 (sorry for the delay)
Yes, your analysis is right. about the fromUniquePair difference.
As for it being faster, I doubt it makes much difference.

That being said, for the getting started experience, from+join is clearer, so let's figure out why that doesn't do what it's suppose to do. I am not seeing what you are seeing. Shall we just jump in a quick meeting sometime to figure it out?

rhuan080 · 2021-04-09T17:41:02Z

@ge0ffrey updated. Thank you for helping me to understand the problem and solve it. :-)

docs/src/main/asciidoc/optaplanner.adoc

ge0ffrey · 2021-04-12T14:58:03Z

I think this is good to merge. Thanks @rhuan080 !

ge0ffrey · 2021-04-19T13:02:19Z

@gastaldi @gsmet Can we get this merged?

Signed-off-by: Rhuan Rocha <rhuan080@gmail.com>

gsmet

I squashed the commits, let's wait for CI and merge.

quarkus-bot bot added the area/documentation label Mar 13, 2021

ge0ffrey suggested changes Mar 15, 2021

View reviewed changes

ge0ffrey reviewed Mar 15, 2021

View reviewed changes

rhuan080 commented Mar 17, 2021

View reviewed changes

gastaldi reviewed Apr 9, 2021

View reviewed changes

docs/src/main/asciidoc/optaplanner.adoc Outdated Show resolved Hide resolved

ge0ffrey approved these changes Apr 12, 2021

View reviewed changes

Fixing the optaplanner guide

8d5cd6e

Signed-off-by: Rhuan Rocha <rhuan080@gmail.com>

gsmet force-pushed the fix_optaplanner_guide branch from 111fb7b to 8d5cd6e Compare May 11, 2021 14:30

gsmet approved these changes May 11, 2021

View reviewed changes

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 11, 2021

gastaldi merged commit 7f12038 into quarkusio:main May 11, 2021

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 11, 2021

quarkus-bot bot added this to the 2.0 - main milestone May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the optaplanner guide #15702

Fixing the optaplanner guide #15702

rhuan080 commented Mar 13, 2021

gsmet commented Mar 14, 2021

ge0ffrey left a comment •

edited

ge0ffrey Mar 15, 2021

rhuan080 Mar 15, 2021

ge0ffrey Mar 16, 2021 •

edited

rhuan080 Mar 16, 2021

rhuan080 Mar 17, 2021 •

edited

rhuan080 left a comment •

edited

rhuan080 Mar 16, 2021

ge0ffrey commented Mar 17, 2021 •

edited

rhuan080 commented Mar 17, 2021

rhuan080 commented Mar 19, 2021 •

edited

ge0ffrey commented Apr 8, 2021

rhuan080 commented Apr 9, 2021

ge0ffrey commented Apr 12, 2021

ge0ffrey commented Apr 19, 2021

gsmet left a comment

Fixing the optaplanner guide #15702

Fixing the optaplanner guide #15702

Conversation

rhuan080 commented Mar 13, 2021

gsmet commented Mar 14, 2021

ge0ffrey left a comment • edited

Choose a reason for hiding this comment

ge0ffrey Mar 15, 2021

Choose a reason for hiding this comment

rhuan080 Mar 15, 2021

Choose a reason for hiding this comment

ge0ffrey Mar 16, 2021 • edited

Choose a reason for hiding this comment

rhuan080 Mar 16, 2021

Choose a reason for hiding this comment

rhuan080 Mar 17, 2021 • edited

Choose a reason for hiding this comment

rhuan080 left a comment • edited

Choose a reason for hiding this comment

rhuan080 Mar 16, 2021

Choose a reason for hiding this comment

ge0ffrey commented Mar 17, 2021 • edited

rhuan080 commented Mar 17, 2021

rhuan080 commented Mar 19, 2021 • edited

ge0ffrey commented Apr 8, 2021

rhuan080 commented Apr 9, 2021

ge0ffrey commented Apr 12, 2021

ge0ffrey commented Apr 19, 2021

gsmet left a comment

Choose a reason for hiding this comment

ge0ffrey left a comment •

edited

ge0ffrey Mar 16, 2021 •

edited

rhuan080 Mar 17, 2021 •

edited

rhuan080 left a comment •

edited

ge0ffrey commented Mar 17, 2021 •

edited

rhuan080 commented Mar 19, 2021 •

edited