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
Delay reading from job store on repeated errors #116
Conversation
Hi @whilemitja , This contribution falls under the conditions described under the heading "Contributions which do require a full Contributor’s License Agreement (CLA)" here: http://www.quartz-scheduler.org/community/contribute.html We very much appreciate your contribution, and request that you submit a signed agreement. Thanks! |
CLA submitted.. |
@jhouserizer any update on this? |
@@ -27,6 +27,8 @@ | |||
import org.quartz.SchedulerException; | |||
import org.quartz.Trigger; | |||
import org.quartz.Trigger.CompletedExecutionInstruction; | |||
import org.quartz.impl.jdbcjobstore.JobStoreSupport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direct dependencies on a given JobStore implementation is running afoul of intended abstraction and isolation of the code organization / design.
I think we need to consider a new abstraction for this fix that doesn't create a direct dependency on JobStoreSupport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding a method to the JobStore interface (which we can do with 2.3.0 - but not 2.2.x) named something like "getAcquireRetryDelay()" and the implementation on JobStoreSupport could return the value of dbRetryInterval, and the implementation on RAMJobStore could return 50 or something small, and etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it more like an integration-with, rather than dependency-on, but ok, will remove.
interval = 250 * acquiresFailed; | ||
} | ||
|
||
if (interval > 5000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the dbRetryInterval is 15 seconds, this will reduce the delay to 5 seconds. I wonder if we'd prefer to keep the 15 seconds? I understand that in the "else" case above, where 250 is multiplied by aquiresFailed that the interval could get huge, and therefore need to be constrained to a maximum as is being done here, but maybe that should only apply in the "else" case above.
Ah, now I remember what the initial trigger for pulling in JobStoreSupport was - (at least) one of the tests sets the retry interval to something very small (5 or 50ms), then fails if the test takes too long to finish.. With the new/extra delays, the test indeed took too long, and so I thought it'd be better to take the retry interval into account rather than relaxing the test's time limit.. I see that 2.3.0 release is planned for April 16th, which is fairly soon, so I'll go ahead and add that method you suggested to JobStore.. |
This looks nice. Thanks again for your contribution! |
could this change be backported to the 2.2.x series as this causes us a lot of trouble in production? |
Any update on this? When will version 2.3 be released? |
Hi all, in . |
Praveen , Can you tell me where exactly you did the changes, which file? |
Hello,
this change helps with issue #71, which is occasionally causing us some trouble in production.
If it's acceptable, it'd be great if we could also get it backported to the 2.2.x series, so that we don't have to wait until 2.3 is finished.. I don't know how that is done, though..
If it's not acceptable, please let me know of a better approach and I'm happy to change the implementation..