-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Reset trigger state from error state #25
Conversation
+1 |
Curious what the status of this is as it has been a few months |
+1 |
Hi @marwtki, 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! |
+1. Good Feature. |
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.
Thank you so much for the contribution!
Please review the items I have added. Also it would be nice if we can have some tests to cover this new API.
@@ -0,0 +1,6 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
IDE generated files should not be checkin
* @see TriggerState#NONE | ||
*/ | ||
public void resetTriggerState(final TriggerKey triggerKey) throws JobPersistenceException { | ||
executeWithoutLock( |
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 think we should use executeInLock instead.
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.
agreed.
public void resetTriggerState(Connection conn, TriggerKey key) | ||
throws JobPersistenceException { | ||
try { | ||
getDelegate().updateTriggerState(conn, key, STATE_WAITING); |
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.
For audit sake, I think we should retrieve the old value and LOG it before reset new value.
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.
agreed.
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 also wonder if this should only work for triggers in ERROR state. (vs. acquired, blocked, paused).
return; | ||
} | ||
tw.state = TriggerWrapper.STATE_WAITING; | ||
applyMisfire(tw); |
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.
- Why are we calling applyMisfire here? I don't think resetTriggerState should imply misfire.
- Also, for RAMJobStore, the initial state is set to null, so I think reset it to null instead of STATE_WAITING is more consistent. As long as it is added to the timeTriggers set is fine.
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.
on point 1. I agree the call is redundant/superfluous. no real harm, no real gain.
on point 2. I agree that consistency would be good - it may make a difference at some point in the future.
@@ -1589,6 +1589,19 @@ public TriggerState getTriggerState(TriggerKey triggerKey) throws SchedulerExcep | |||
|
|||
/** | |||
* <p> | |||
* Reset the current state of the identified <code>{@link Trigger}</code>. |
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.
Should we clarify what value will the trigger state be after reset?
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 agree - I wonder if the method name should also be more explicit about what it does / is for. Like what if you call this method on a trigger that is in BLOCKED state (and for proper reason is in blocked state)?
|
||
tw.setState(TriggerState.WAITING, terracottaClientId, triggerFacade); | ||
|
||
applyMisfire(tw); |
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.
Again, why we need to call applyMisfire?
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.
It is superfluous - will get detected and handled later if the misfire condition exists, but it is also likely that the trigger has actually misfired, so there is a small argument for eagerly updating it here (and the other locations).
@jhouserizer I have submitted a signed agreement. Thank you! |
CLA received. Thank you for contributing! |
Another finding: simply resetting to "WAITING" state is a bit naive. If the trigger group is paused, it should go to PAUSED, for example. I think we need these changes:
|
@marwtki , would you have a chance in the next few days to make some updates to this PR (based upon comments here) - or should I take your changes, edit, and create a new PR? (I say "few days" because we want to release 2.3.0 in a week). |
@jhouserizer, I'm not sure @marwtki is available to help with this anymore. She wrote it when working at Lucid last summer, but is no longer working with us. A couple months ago I applied @zemian's feedback and made a PR to her branch, but it was never merged. What would be easiest is for me to fork, apply the changes and feedback, and then I can open a new PR. I can do that either today or tomorrow if that works for you. If you would prefer to take the changes and make a pr yourself, feel free. Whatever is easiest for you. |
Thanks @jjudd ! it would be appreciated if you did that. The unfortunate thing is that we'll then also need a CLA from you (see comment above from Oct 11, 2016 for instructions) - hopefully would only take you a few minutes to do. (Or remind me if you have already done one in the past). |
@jhouserizer, I have a branch here with the feedback applied: jjudd/quartz@8fec020 I'm finding out if anyone has signed the CLA and can contribute the changes. Otherwise, I'm not sure I will be able to have the CLA reviewed and submitted before your release. If that is the case, feel free to use that branch as inspiration for your edits when making a new PR. |
Thanks for the updates. I'll take these changes and modify them to account for remaining comments above. |
See new PR #125 . Thanks to you all for the contributions. |
When our triggers get into an error state, I want to reset the trigger state after fixing the underlying problem. I could modify the database directly but would rather go through an official API.