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

BUG: Doorman runner extensibility #287

Merged
merged 1 commit into from
Jan 17, 2021

Conversation

mfendeksilverstripe
Copy link
Contributor

@mfendeksilverstripe mfendeksilverstripe commented Mar 16, 2020

Doorman runner extensibility

This is a follow up to #279

Details

  • DefaultRules now uses named keys so project level configuration override is possible (typically we want to replace or remove the default rule)
  • new configuration max_ticks - this limits the number of loops (disabled by default, backwards compatible)
  • new configuration tick_interval - this makes the hardcoded interval of sleep(1) configurable (defaults to 1, backwards compatible)
  • new configuration child_runner - this makes the hardcoded name of the dev task which is used to run the child processes configurable (defaults to ProcessJobQueueChildTask , backwards compatible)
  • queue runner is now checking for maintenance lock
  • exposed list of all job statuses as a new public function

Related issues

#288
#297
#296

@emteknetnz
Copy link
Collaborator

emteknetnz commented Mar 17, 2020

@mfendeksilverstripe removing the protected function getNextJobDescriptorWithoutMutex() would technically make this a breaking change, meaning we need to release this as a major version which may not happen for a long time

Just a quick recap:

  • add/modify/remove privates = patch (best)
  • add public/protected = minor
  • delete/modify-signature public/protected = major

are you able to refactor this PR so that we could release this as either a minor or even patch?

@mfendeksilverstripe
Copy link
Contributor Author

@emteknetnz Thanks for the heads up, I pushed the following fixes:

  • restored getNextJobDescriptorWithoutMutex method for backwards compatibility
  • exposed the list of all job statuses as a new public function
  • added initialising_state_ttl setting
  • minor code cleanup

Please review ;-)

@mfendeksilverstripe
Copy link
Contributor Author

Pushed up a fix for job locking in the case of the job processing hitting memory or execution limits.

@mfendeksilverstripe
Copy link
Contributor Author

@Cheddam Could you please have a look at this one?

@mfendeksilverstripe
Copy link
Contributor Author

@maxime-rainville Could you please find some time to review this?

@nicole-ashley
Copy link

@mfendeksilverstripe Would it be possible to split out the release job lock fix into a separate PR? It's an important fix that's completely separate to Doorman and would be great to have in mainline.

@mfendeksilverstripe
Copy link
Contributor Author

@nikrolls Sure, I can do that ;-)

@nicole-ashley
Copy link

Thanks!

@mfendeksilverstripe
Copy link
Contributor Author

@nikrolls Job lock release changes split into a separate PR

#320

@mfendeksilverstripe mfendeksilverstripe force-pushed the bugfix/doorman-runner branch 2 times, most recently from 1e01f9f to 4e476f3 Compare December 14, 2020 02:11
@Cheddam Cheddam self-assigned this Dec 16, 2020
Copy link

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

Have tested locally in a limited fashion. Setting max_ticks: 1 successfully bails on long-running tasks, but I'm not sure whether they're intended to remain at STATUS_INIT?

src/Tasks/Engines/DoormanRunner.php Outdated Show resolved Hide resolved
src/Tasks/Engines/DoormanRunner.php Outdated Show resolved Hide resolved
@mfendeksilverstripe mfendeksilverstripe changed the title BUG: Doorman runner job locking fix BUG: Doorman runner extensibility Jan 14, 2021
@mfendeksilverstripe
Copy link
Contributor Author

@Cheddam Thanks for feedback, I've pushed up the changes you suggested. Also, the branch is now rebased.

I tested this locally and it's working for me. This is how it looks like:

Screen Shot 2021-01-15 at 11 16 33 AM

Screen Shot 2021-01-15 at 11 17 02 AM

Screen Shot 2021-01-15 at 11 17 08 AM

Screen Shot 2021-01-15 at 11 17 15 AM

Screen Shot 2021-01-15 at 11 17 21 AM

Screen Shot 2021-01-15 at 11 17 26 AM

You may need to run it with this this Doorman fix in place though.

@mfendeksilverstripe mfendeksilverstripe force-pushed the bugfix/doorman-runner branch 2 times, most recently from bdec53d to 8e03795 Compare January 15, 2021 00:05
@mfendeksilverstripe
Copy link
Contributor Author

mfendeksilverstripe commented Jan 15, 2021

@Cheddam It's rebased now, I tested this locally and it's running well for me. All test suites are passing except one, not sure why.

Screen Shot 2021-01-15 at 1 14 35 PM

What's the difference between the nightly test suite compared to others?

@Cheddam
Copy link

Cheddam commented Jan 15, 2021

Nightly in this context refers to PHP 8. We've just merged #334, so this failure has been resolved - one more rebase should get your build green 😅

@mfendeksilverstripe
Copy link
Contributor Author

Thanks for update @Cheddam . It looks like #334 needs to be merged into 4 before I can rebase. Please let me know when this is done. :)

@Cheddam
Copy link

Cheddam commented Jan 15, 2021

@mfendeksilverstripe The merge up has been completed now :)

@mfendeksilverstripe
Copy link
Contributor Author

@Cheddam I've rebased the branch and the tests are now green. Tested this again on my local setup and it's all looking good. Just for reference this is my local test setup:

{
    "name": "silverstripe/installer",
    "type": "silverstripe-recipe",
    "description": "The SilverStripe Framework Installer",
    "require": {
        "php": ">=7.1.0",
        "silverstripe/recipe-plugin": "^1.2",
        "silverstripe/recipe-cms": "^4.7",
        "silverstripe-themes/simple": "~3.2.0",
        "silverstripe/login-forms": "4.1.2@stable",
        "symbiote/silverstripe-queuedjobs": "dev-bugfix/doorman-runner as 4.6.0",
        "silverstripe-terraformers/gridfield-rich-filter-header": "^2.1"
    },
    "require-dev": {
        "phpunit/phpunit": "^5.7",
        "sminnee/phpunit-mock-objects": "^3.4.5"
    },
    "extra": {
        "resources-dir": "_resources",
        "project-files-installed": [
            "app/.htaccess",
            "app/_config.php",
            "app/_config/database.yml",
            "app/_config/mimevalidator.yml",
            "app/_config/mysite.yml",
            "app/src/Page.php",
            "app/src/PageController.php"
        ],
        "public-files-installed": [
            ".htaccess",
            "index.php",
            "web.config"
        ]
    },
    "config": {
        "process-timeout": 600
    },
    "prefer-stable": true,
    "minimum-stability": "dev",
    "repositories": {
        "symbiote/silverstripe-queuedjobs": {
            "type": "git",
            "url": "https://github.com/silverstripe-terraformers/silverstripe-queuedjobs.git"
        }
    }
}

---
Name: app-queue
After: '#queuedjobsettings'
---
Symbiote\QueuedJobs\Services\QueuedJobService:
  lock_file_enabled: true
  lock_file_path: '/public/queue-lock'
  max_init_jobs: 100
  memory_limit: 128m
  use_shutdown_function: false

SilverStripe\Core\Injector\Injector:
  Symbiote\QueuedJobs\Services\QueuedJobService:
    properties:
      queueRunner: '%$Symbiote\QueuedJobs\Tasks\Engines\DoormanRunner'

Copy link

@Cheddam Cheddam left a comment

Choose a reason for hiding this comment

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

Thanks for all your hard work on this one @mfendeksilverstripe!

@Cheddam Cheddam merged commit 00938b3 into symbiote:4 Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants