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.

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

Daemon Mode Bug fixed #89

Closed
wants to merge 4 commits into from
Closed

Daemon Mode Bug fixed #89

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 29, 2015

The solverScope time parameters do not reset when solver starts again after a time termination.
It is fixed now. See the changes in the CloudBalancingDaemon Test.

Detailed description can be found here.
https://groups.google.com/forum/#!topic/optaplanner-dev/Rj1d2rGXa_k

@@ -183,12 +183,10 @@ public final void solve(Solution planningProblem) {
public void outerSolvingStarted(DefaultSolverScope solverScope) {
solving.set(true);
basicPlumbingTermination.resetTerminateEarly();
solverScope.setStartingSystemTimeMillis(System.currentTimeMillis());
solverScope.setEndingSystemTimeMillis(null);
solverScope.setStartingSolverCount(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

startingSolverCount should only reset in the outerSolving, because it is used to determine if a solver is "started" or "restarted", see DefaultSolver#solvingStarted().

@ge0ffrey
Copy link
Contributor

ge0ffrey commented Jun 2, 2015

Thanks for the PR. I am merging & reviewing & fixing now, will take a while I think, because there is a deeper problem.

@@ -183,12 +183,10 @@ public final void solve(Solution planningProblem) {
public void outerSolvingStarted(DefaultSolverScope solverScope) {
solving.set(true);
basicPlumbingTermination.resetTerminateEarly();
solverScope.setStartingSystemTimeMillis(System.currentTimeMillis());
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that resets are needed, for time terminations, for timegradient, etc.
But don't we need to track the outerStarting time too, for some functionality? Investigating now.

Copy link
Author

Choose a reason for hiding this comment

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

Yes the startingSovlerCount should be placed in the outerSolverStarted. Agreed that the outerSolverStarting time could be useful. Because in multi-tenance application, each daemon mode solver will hog up a cpu core and therefore is resource consuming (more $$ charging for tenants). Some tenants may want to kill the always-on solver after the operation hours (e.g. 9am to 9pm for a vehicle routing application). Then the optaplanner execution server cluster can serve more customers with less cpu cores with the help of a job dispatching layer. The outerSolvingTime can be used to indicate how long the "solving application" has been running and the solver can stop restarting the solver in daemon once the time termination config has been met. private boolean checkProblemFactChanges(){ if (termination.isDaemonModeTerminated(System.currentTimeMillis - outerSolverStaringTime){ return false; }…..…..}private boolean checkProblemFactChanges() {  Original Message  Sender: Geoffrey De Smetnotifications@github.comRecipient: droolsjbpm/optaplanneroptaplanner@noreply.github.comCcisaaczhangadventurer18821688@qq.comDate: Tuesday, Jun 2, 2015 17:41Subject: Re: [optaplanner] Daemon Mode Bug fixed (#89)In optaplanner-core/src/main/java/org/optaplanner/core/impl/solver/DefaultSolver.java:

@@ -183,12 +183,10 @@ public final void solve(Solution planningProblem) {
public void outerSolvingStarted(DefaultSolverScope solverScope) {
solving.set(true);
basicPlumbingTermination.resetTerminateEarly();

  •    solverScope.setStartingSystemTimeMillis(System.currentTimeMillis());
    

Agreed that resets are need, for time terminations, for timegradient, etc.
But don't we need to track the outerStarting time too, for some functionality? Investigating now.

—Reply to this email directly or view it on GitHub.

@ge0ffrey
Copy link
Contributor

ge0ffrey commented Jun 2, 2015

This also affects the average score count reported in the log. I am going to make that only account the ACC of the last solver start-end cycle. In 6.2.0.Final it reported a too low ACC of the entire solver run if the daemon mode was blocking (well too low for all practical interpretation use anyway).

@ge0ffrey
Copy link
Contributor

ge0ffrey commented Jun 2, 2015

The good news:

  • It's fixed for 6.3.0.Beta1 along with a few other improvements related to real-time planning
    http://github.com/droolsjbpm/optaplanner/compare/89a6a45...18b4e78
  • I tried your test locally and it works. Thanks for sending this PR with the unit test, as it made it clear what the exact problem was and what was your desired behaviour :)

The bad news:

  • I didn't merge your PR, but used it an inspiration.
  • I can't add the unit test, because 37s to run 1 test is 32 seconds over the limit. It would make our entire test suite too slow, which means people wouldn't run our tests any more. (it's already above 5 minutes in total and that's already too long to run on every commit). I 've considered adding it as a turtle test (activated with -DturtleTest=true) but I also fear it might be cause false negatives (our jenkins slaves are notoriously slow on occasion and might take 60 seconds to do something that you'd expect to happen in less than 1 second). However, I see now better way of writing that test, without expecting that something will happen in a reasonable amount of time. I am choosing the lesser of 2 evils here and leaving it out...

Keep the PR's coming :)

@ge0ffrey ge0ffrey closed this Jun 2, 2015
@ghost
Copy link
Author

ghost commented Jun 2, 2015

Great!Users are advised to run a long term solving running a turtle test that walks through the whole life cycle of their customized solver though ( building a solver, get it running till it waits in daemon mode, calls all the problemFactChange interface defined by the applications).In my limited experience, it is especially necesary to run such a long term test if the user have created some complicated  moveFactoryIterator, filters and problemfactChange interfaces.  Original Message  Sender: Geoffrey De Smetnotifications@github.comRecipient: droolsjbpm/optaplanneroptaplanner@noreply.github.comCcisaaczhangadventurer18821688@qq.comDate: Tuesday, Jun 2, 2015 23:40Subject: Re: [optaplanner] Daemon Mode Bug fixed (#89)The good news:

It's fixed for 6.3.0.Beta1 along with a few other improvements related to real-time planning
http://github.com/droolsjbpm/optaplanner/compare/89a6a45...18b4e78

I tried your test locally and it works. Thanks for sending this PR with the unit test, as it made it clear what the exact problem was and what was your desired behaviour :)

The bad news:

I didn't merge your PR, but used it an inspiration.
I can't add the unit test, because 37s to run 1 test is 32 seconds over the limit. It would make our entire test suite too slow, which means people wouldn't run our tests any more. (it's already above 5 minutes in total and that's already too long to run on every commit). I 've considered adding it as a turtle test (activated with -DturtleTest=true) but I also fear it might be cause false negatives (our jenkins slaves are notoriously slow on occasion and might take 60 seconds to do something that you'd expect to happen in less than 1 second). However, I see now better way of writing that test, without expecting that something will happen in a reasonable amount of time. I am choosing the lesser of 2 evils here and leaving it out...

Keep the PR's coming :)

—Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants