Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add job states and job owners transparently, for plugin needs etc. #1931

Closed
wants to merge 11 commits into from

Conversation

narration-sd
Copy link
Contributor

@narration-sd narration-sd commented Aug 19, 2017

Brandon, this is the first stage of adding state to the Craft job queue, as we've discussed. I've used that word to avoid colliding with other uses of 'status' within the queue.

I'd like to hear if there's anything you'd like changed of the form of this before I complete it. That completion step will bring use of the 'owner' field I've also added to craft_queue in the migration.

The purpose of 'owner' is to give an identifier so operations like the Maintenance panel kill-all-own-jobs button on my database agent can clear matters up during integration tests etc. without hitting anything else that's going on -- uses like that. Intent is to be shorter and more definite than parsing Description strings as I'm doing for the moment.

Tested so far and works nicely -- Job states are flowing through on the CP HUD, without any mods there as you'll see.

p.s. if you wonder where my work is, really I use Bitbucket, and anyway its so far private repos.

@narration-sd
Copy link
Contributor Author

Pic of that screen with the Clear All Tasks button so you can see what I mean by that.

The plugin is meant to be self-educating, for someone who has to come find it a year later without training if the source database goes down, etc. -- that's why it and others are so wordy.

clear-all-button-screen

@narration-sd
Copy link
Contributor Author

narration-sd commented Aug 20, 2017

Ok, doing while it's fresh in mind, I've completed this, by adding handling of the job owner property. The purpose of owner is to allow per-owner operations on the job queue, such as removing own jobs, as I may do for various reasons under fault.

Both owner and state are entirely optional, for jobs that want to use them, and others don't need code them or otherwise to consider their presence.

Because there wasn't an interface call invoved, I was able to implemented owner using reflection, which avoids any changes to Craft-owned or third-party jobs. as the default state property for setProgress() also provides.

All seems to work in my testing. I have two different jobs of complexity in the Standards Agent plugin, and I see that Craft's fixup jobs are running as usual after I post Entries, without having required changes. There are big flurries of work, with 50-400 Entries involved, so I think we're ok unless you think of or notice something.

I admit to having scoped out the Garnish HUD pretty thoroughly again first, also to solve an issue, but was able to design these additions to be useful without touching it, thus as simply as possible. It is very nice to see those states fly by, and to have solidly dependable identity in place for job manipulations. Hope you like it, and cheers...

@narration-sd
Copy link
Contributor Author

narration-sd commented Aug 20, 2017

...I see mystery (conflicting) messages from Scrutinizer this time, now disappeared, so if it's anything but weekend matters, you can tell me...(!)

@narration-sd narration-sd changed the title Jobstates Add job states and job owners transparently, for plugin needs etc. Aug 24, 2017
@narration-sd
Copy link
Contributor Author

Use case for owner filed portion of this PR:

  • You have a plugin using Jobs
  • you have a scheduled Job running
  • Before it completes, the site php server or container reboots due to automated security kernel upgrade or otherwise goes down, say due to VPS server farm admin.
  • You now have a non-failed (because no appropriate Exception made it through) job in the queue, which will prevent others from running
  • Your solution similarly as with Craft 2 tasks should be to queue->remove() the job/s, but you should do that only for your own jobs, not anyone else's.
  • thus you require the owner field, as provided by this PR

Other use cases for the owner filed include an integration screen on a plugin, where remote database setup is validated, and failures can cause extra hung tasks, thus you need a removal button to clear the task queue, which should equally be safetied to your own tasks via owner.

One use case also for the status field portion of the PR:

  • you have a plugin using Jobs
  • you have a multi-step Job, where one or more steps last reasonable human time
  • you want to indicate the step in progress, the state, to CP observers
  • thus you require the state field, in this PR, so that you can reflect this in the HUD message

@narration-sd
Copy link
Contributor Author

narration-sd commented Oct 26, 2017

(nicely expressing so lonely, this pr)
https://www.youtube.com/watch?v=Ydfpk97IzMA

@narration-sd
Copy link
Contributor Author

When I was bringing the PR base up to dev-develop @ RC4, I checked here to be sure tests were ok.

Looking around, I realized I hadn't given you a picture of what the job states portion of this PR is used for, so remedying that by attached. You

What you see on the HUD is a stage in clearing the Craft Entries created by the agent from monitoring a remote database. This is safe because the agent will rebuild them, and might be done if there's been some kind of hiccup, or to start fresh. Because I can set states, the number of records counts down.

Similar things happen across the three stages of checking, comparing, and loading any deltas in the remote database state, which goes off at settable intervals from a cron lrunning a console command.

I think the job owner part is already explained above, It's to allow automatically managing out any stuck tasks, say due to a server reboot at an inconvenient time -- safely limited to only my thus-identified tasks. That's what gives long term reliability to the agent, which has been running on C2 to base a furniture safety etc. standards organization in England for two years.

removing-at-250

@brandonkelly brandonkelly added authoring ✍️ features related to author experience and removed ux labels Feb 26, 2018
@narration-sd
Copy link
Contributor Author

narration-sd commented Sep 13, 2018

@brandonkelly I presume you'd like me to abandon this.

I don't know what was the matter with it that a year later there's no response. I kept up the craft 3 match for quite a while.

Is it that you didn't like the feature of queue task state, labelling this progress on the HUD indicator? This seemed quite attractive and useful to me in the implementation, and would have been expected, if the client for the original database agent had ever returned for the upgrade he once expected.

Anyway, it wouldn't be for free now, even though the plugin conversion was my lesson in Craft 3 once.

So, I'll plan to withdraw the PR, once you've had a chance to see this. I've several times avoided even thinking of doing one for features --- both because of this episode, and because the use of events and behaviors, for which I appreciate your opening the doors, both personally and for the project.

With those, I've managed all the intricate interactions of the Live Vue effort, and it really is ready to go out the door as of this evening, all the add-on layers and branches concluding with Sites. A feature I'm working on with Mark Huot isn't singular to Live Vue, but to Vue etc. in general, and can come in its time for a narrow but important case.

Documentation, next, which has driven the last efforts to make them straightforward enough to explain, and soft beta to see that the taming is good enough that people will be able to sensibly use it.

Running two parallel complex systems, and keeping them in line with each other for so many cases, what fun ;)

Clive

@narration-sd
Copy link
Contributor Author

narration-sd commented Sep 14, 2018

I just merged latest develop - looks like no problems.

But -- I am going to withdraw the pull request, probably this weekend, so that you have a clean slate heading out to Berlin, and for that matter, 3.1.

I'll keep it around as a repo, in case specific need revives.

Cheers, @brandonkelly & @angrybrad

Clive

@narration-sd
Copy link
Contributor Author

@brandonkelly @angrybrad Ok, going once, going twice, gone....

Poof!

@brandonkelly
Copy link
Member

Hey @narration-sd sorry for the lack of response here. We need to review all the changes in every PR, and due to the size of this one, we just needed to slot it in alongside other feature development, when we’re ready to implement a feature like this. Realistically it will probably be just as fast for us to implement the feature ourselves as it would be to review this, so it’s fine that you closed it. We will still keep it in mind when we’re ready to look at this feature.

@narration-sd
Copy link
Contributor Author

narration-sd commented Sep 17, 2018 via email

@brandonkelly brandonkelly removed the authoring ✍️ features related to author experience label May 16, 2019
@brandonkelly brandonkelly added ux 😄 features related to user experience enhancement improvements to existing features labels May 16, 2019
@brandonkelly brandonkelly added this to the 3.2 milestone May 16, 2019
brandonkelly added a commit that referenced this pull request May 16, 2019
@brandonkelly
Copy link
Member

Finally got around to implementing a version of this for the next 3.2 Alpha release.

@narration-sd
Copy link
Contributor Author

Great stuff; just happened to spot it. That project is mothballed, but am sure this will be useful in futures...thanks, on all of our behalf!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement improvements to existing features ux 😄 features related to user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants