Reset trigger state from error state by margkam · Pull Request #25 · quartz-scheduler/quartz

margkam · 2016-06-13T16:58:14Z

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.

disaacson · 2016-06-14T17:00:18Z

+1

jjudd · 2016-08-18T21:36:15Z

Curious what the status of this is as it has been a few months

alangshall · 2016-08-29T22:38:40Z

+1

jhouserizer · 2016-10-11T14:00:59Z

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!

ramaraochavali · 2016-10-17T04:06:30Z

+1. Good Feature.

zemian

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.

zemian · 2016-10-18T04:03:42Z

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>


IDE generated files should not be checkin

zemian · 2016-10-18T04:06:53Z

+     * @see TriggerState#COMPLETE
+     * @see TriggerState#ERROR
+     * @see TriggerState#NONE
+     */


I think we should use executeInLock instead.

zemian · 2016-10-18T04:08:08Z

+    }
+
+    public void resetTriggerState(Connection conn, TriggerKey key)
+            throws JobPersistenceException {


For audit sake, I think we should retrieve the old value and LOG it before reset new value.

I also wonder if this should only work for triggers in ERROR state. (vs. acquired, blocked, paused).

zemian · 2016-10-18T04:14:23Z

+            // does the trigger exist?
+            if (tw == null || tw.trigger == null) {
+                return;
+            }


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.

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.

zemian · 2016-10-18T04:16:59Z

        return resources.getJobStore().getTriggerState(triggerKey);
    }

+    /**


Should we clarify what value will the trigger state be after reset?

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)?

zemian · 2016-10-18T04:19:29Z

+      // does the trigger exist?
+      if (tw == null) { return; }
+
+      tw.setState(TriggerState.WAITING, terracottaClientId, triggerFacade);


Again, why we need to call applyMisfire?

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).

margkam · 2016-10-19T18:23:36Z

@jhouserizer I have submitted a signed agreement. Thank you!

jhouserizer · 2016-10-19T22:46:59Z

CLA received. Thank you for contributing!

…er#25)

jhouserizer · 2017-04-08T12:47:24Z

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:

rename API method name to resetTriggerFromErrorState - in order to be clear that it only works on triggers in that state, not other states
the doc should indicate that the trigger will be set to WAITING or PAUSED state depending upon which is correct (whether related trigger group or job group is paused)
update the code in the jobstores to actually behave this way (only do something if current state is error, and then properly choose the new state)
** See code in JobStoreSupport.triggeredJobComplete(), JobStoreSupport.triggerFired() for reference of how some trigger state transitions are handled.

jhouserizer · 2017-04-08T14:01:09Z

@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).

jjudd · 2017-04-08T15:17:50Z

@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.

jhouserizer · 2017-04-08T15:22:10Z

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).

jjudd · 2017-04-10T02:47:12Z

@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.

…er#25)

Remote trigger pr feedback

jhouserizer · 2017-04-12T01:55:41Z

Thanks for the updates. I'll take these changes and modify them to account for remaining comments above.

jhouserizer · 2017-04-15T00:18:28Z

See new PR #125 .

Thanks to you all for the contributions.

Add resetTriggerFromErrorState functionality from #25 and #45

Reset trigger state from error state

e7bf40b

margkam mentioned this pull request Aug 18, 2016

Allow a user to see and clear trigger state lucidsoftware/piezo#45

Open

zemian added needs review labels Oct 16, 2016

zemian requested changes Oct 18, 2016

View reviewed changes

zemian added need rework and removed needs review labels Oct 18, 2016

jhouserizer added CLA accepted and removed CLA pending labels Oct 19, 2016

disaacson mentioned this pull request Jan 9, 2017

Trigger state issue 45 lucidsoftware/piezo#55

Closed

jjudd pushed a commit to jjudd/quartz that referenced this pull request Feb 10, 2017

Add pull request feedback for resetting trigger state (quartz-schedul…

8fec020

…er#25)

jhouserizer added this to the 2.3.0 milestone Apr 6, 2017

Margaret Watkins and others added 4 commits April 10, 2017 14:09

Reset trigger state from error state

dc99c9c

Add pull request feedback for resetting trigger state (quartz-schedul…

b846d57

…er#25)

Merge branch 'master' into remote-trigger-pr-feedback

28cd6c7

Merge pull request #1 from jjudd/remote-trigger-pr-feedback

9bbde44

Remote trigger pr feedback

jhouserizer mentioned this pull request Apr 15, 2017

Add resetTriggerFromErrorState functionality from #25 and #45 #125

Merged

jhouserizer closed this Apr 15, 2017

jhouserizer removed this from the 2.3.0 milestone Apr 15, 2017

jhouserizer added a commit that referenced this pull request Apr 15, 2017

Merge pull request #125 from jhouserizer/reset_trigger_error_state

4509c15

Add resetTriggerFromErrorState functionality from #25 and #45

Conversation

margkam commented Jun 13, 2016

Uh oh!

disaacson commented Jun 14, 2016

Uh oh!

jjudd commented Aug 18, 2016

Uh oh!

alangshall commented Aug 29, 2016

Uh oh!

jhouserizer commented Oct 11, 2016

Uh oh!

ramaraochavali commented Oct 17, 2016

Uh oh!

zemian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

margkam commented Oct 19, 2016

Uh oh!

jhouserizer commented Oct 19, 2016

Uh oh!

jhouserizer commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhouserizer commented Apr 8, 2017

Uh oh!

jjudd commented Apr 8, 2017

Uh oh!

jhouserizer commented Apr 8, 2017

Uh oh!

jjudd commented Apr 10, 2017

Uh oh!

jhouserizer commented Apr 12, 2017

Uh oh!

jhouserizer commented Apr 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jhouserizer commented Apr 8, 2017 •

edited

Loading