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.

Sign up for GitHub

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

Jump to bottom

Move priority setting to schedule.json #161

Merged
merged 4 commits into from
Jun 23, 2019
Merged

Move priority setting to schedule.json #161

merged 4 commits into from
Jun 23, 2019

Conversation

Digenis
Copy link
Member

@Digenis Digenis commented Jul 22, 2016 •
edited
Loading

Continued from #144 (comment)

The arguments passed from component to component contain arguments addressed to different components, arguments addressed to the crawl process all in a single dictionary. The components shouldn't be poping keys from the dictionary containing the crawl arguments.

Priorities are of interest to schedulers and queues, not processes.
That's why I make the setting a positional argument in their method calls.

@redapple redapple mentioned this pull request Jul 22, 2016
Closed
@Digenis Digenis changed the title [WIP] Move priority setting to schedule.json Move priority setting to schedule.json Nov 20, 2017
@Digenis Digenis added this to the 1.3.0 milestone Nov 20, 2017
@Digenis Digenis force-pushed the move_priority_setting_to_schedule.json branch from 448284c to c131c0b Compare April 22, 2019 17:38
@codecov
Copy link

codecov bot commented Apr 22, 2019 •
edited
Loading

Codecov Report

Merging #161 into master will decrease coverage by 0.11%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
- Coverage   68.48%   68.37%   -0.12%     
==========================================
  Files          17       17              
  Lines         860      860              
  Branches      104      104              
==========================================
- Hits          589      588       -1     
- Misses        241      242       +1     
  Partials       30       30
Impacted Files Coverage Δ
scrapyd/webservice.py 29.41% <0%> (-0.25%) ⬇️
scrapyd/interfaces.py 100% <100%> (ø) ⬆️
scrapyd/scheduler.py 100% <100%> (ø) ⬆️
scrapyd/spiderqueue.py 95.23% <100%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ceed69...cd35bd4. Read the comment docs.

@Digenis
Copy link
Member Author

Digenis commented Apr 22, 2019

Needs testing with custom ws, scheduler and queue.
Also, changelog.

@Digenis Digenis force-pushed the move_priority_setting_to_schedule.json branch 2 times, most recently from 9d709b7 to 701c609 Compare June 20, 2019 19:43
@Digenis
Copy link
Member Author

Digenis commented Jun 20, 2019

TODO (later): document testing custom components with zope.interface.verify

@Digenis Digenis force-pushed the move_priority_setting_to_schedule.json branch from 701c609 to 221910e Compare June 21, 2019 06:27
Digenis added 4 commits June 21, 2019 09:30
@Digenis
Move priority setting to schedule.json webservice
f80e0b0 
Priorities are of interest to schedulers and queues, not processes.
@Digenis
add priority arg to scheduler & spiderqueue ifaces
554e92b
@Digenis
update scheduler/queue/poller tests w/priority arg
bb91753 
Tests must fail for customized scheduler/queue that uses the old argspec
@Digenis
pass priority as kwarg for compat with prev iface
cd35bd4 
This is for those who may have written their own scheduler or queue.
Their code will not break but zope.interface.verify, if used in tests
will throw exceptions to warn them.
@Digenis Digenis force-pushed the move_priority_setting_to_schedule.json branch from 221910e to cd35bd4 Compare June 21, 2019 06:35
@Digenis
Copy link
Member Author

Digenis commented Jun 21, 2019

Will merge if no comment.
Review welcome.

@my8100
Copy link
Collaborator

my8100 commented Jun 21, 2019 •
edited
Loading

I have never used the priority option before.
Would try and test it this weekend.

@Digenis
Copy link
Member Author

Digenis commented Jun 21, 2019

It's meant to set the priority of a scheduled run
but only within the project's queue
which makes it useless in most multi-project cases.
Especially when the pending queue never empties
#187 becomes a showstopper.

@my8100
Copy link
Collaborator

my8100 commented Jun 23, 2019

It seems that jobs of project 'TESTING2' would not be scheduled
until there are no pending jobs of project 'TESTING' due to #187,
no matter how high the priority of project 'TESTING2' is.

BTW, should/could we show the priority info in the jobs page,
as well as the API listjobs.json?

@Digenis
Copy link
Member Author

Digenis commented Jun 23, 2019

Makes sense, yes.

@Digenis
Copy link
Member Author

Digenis commented Jun 23, 2019

any comment before merge?

@my8100
Copy link
Collaborator

my8100 commented Jun 23, 2019 •
edited
Loading

I think it’s ready to go.

@Digenis Digenis merged commit 3ff7c1c into scrapy:master Jun 23, 2019
@my8100 my8100 mentioned this pull request Jun 24, 2019
Closed
@my8100
Copy link
Collaborator

my8100 commented Jun 24, 2019

BTW, should/could we show the priority info in the jobs page,
as well as the API listjobs.json?

Implemented in PR #343.

@my8100 my8100 mentioned this pull request Jul 3, 2019
Closed
@jpmckinney jpmckinney mentioned this pull request Sep 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
topic: cleanup
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Digenis @my8100