Skip to content

Added job_type to runworker command#509

Closed
piotrpoz wants to merge 2 commits into
dereuromark:masterfrom
revisual-app:dev-cake5-10.0.0
Closed

Added job_type to runworker command#509
piotrpoz wants to merge 2 commits into
dereuromark:masterfrom
revisual-app:dev-cake5-10.0.0

Conversation

@piotrpoz

Copy link
Copy Markdown

No description provided.

@dereuromark

Copy link
Copy Markdown
Owner

Thanks for the contribution! Unfortunately this PR can't be merged as-is - it looks like a private fork branch (dev-cake5-10.0.0) was pushed against master by mistake. A few blocking issues:

  1. Breaks the worker at runtime. QueueProcessesTable::add() now requires a 3rd argument $jobType, but the only caller (Processor::add() at src/Queue/Processor.php:530) still calls ->add($pid, $key). That's an ArgumentCountError on worker start.
  2. No column for job_type on queue_processes. The validator adds notEmptyString('job_type') and add() inserts 'job_type', but no migration creates that column on the queue_processes table - so it would fail on insert.
  3. The migration targets the wrong table and contradicts the feature. It renames job_type -> job_task on queued_jobs, while the feature is about queue_processes.job_type. It also uses AbstractMigration (should be Migrations\BaseMigration) and has an invalid back-dated timestamp (20191319..., month 13 / day 19).
  4. Entity vs migration contradiction. A job_type property is added to the QueuedJob entity, but the migration removes job_type from queued_jobs.
  5. The README "FORK NOTES" section is fork-internal and not something for upstream.

If the goal is to track the job type on a running process, the right shape would be:

  • a new migration adding a job_type column to queue_processes
  • update the Processor::add() call site to pass it
  • optional validation for the new column

Could you open an issue describing what you're actually after? Happy to help land it properly. Closing this one for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants