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

implement job persistance #359

Closed
wants to merge 1 commit into from
Closed

Conversation

mxdev88
Copy link
Contributor

@mxdev88 mxdev88 commented Nov 20, 2019

Hello,

Could someone pls review this PR about #12?

Thank you

@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #359 (9f04d35) into master (f916d9b) will increase coverage by 2.65%.
The diff coverage is 96.66%.

❗ Current head 9f04d35 differs from pull request most recent head efbdede. Consider uploading reports for the commit efbdede to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
+ Coverage   68.66%   71.32%   +2.65%     
==========================================
  Files          17       18       +1     
  Lines         868      952      +84     
  Branches      104      113       +9     
==========================================
+ Hits          596      679      +83     
  Misses        241      241              
- Partials       31       32       +1     
Impacted Files Coverage Δ
scrapyd/launcher.py 43.58% <66.66%> (-0.17%) ⬇️
scrapyd/jobstorage.py 95.65% <95.65%> (ø)
scrapyd/app.py 88.13% <100.00%> (+1.34%) ⬆️
scrapyd/interfaces.py 100.00% <100.00%> (ø)
scrapyd/sqlite.py 91.24% <100.00%> (+2.35%) ⬆️

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 f916d9b...efbdede. Read the comment docs.

@mxdev88
Copy link
Contributor Author

mxdev88 commented Nov 23, 2019

Anyone to take a first look? @Digenis?

@mxdev88 mxdev88 force-pushed the persist-jobs branch 2 times, most recently from a675326 to e0b79aa Compare November 24, 2019 20:32
@Digenis
Copy link
Member

Digenis commented Dec 31, 2019

Hi, @mxdev88,
thank you for your contribution.

I don't have much time to maintain scrapyd nowadays.
I only took a quick look.

The travis error can be ignored, it's #359.
Overall it seems cleanly implemented
and I guess it will not complicate merging the queues.

@mxdev88
Copy link
Contributor Author

mxdev88 commented Jan 2, 2020

Hi @Digenis,
Great - thanks for the feedback. Let me know if anything else needed prior to merging.

@sluceno
Copy link

sluceno commented Jan 28, 2020

Not an expert on python but it looks good to me.

I will take this as an example for doing similar thing with the Queue so we can have different backends for queueing jobs

@skykery
Copy link

skykery commented Feb 27, 2020

Hope this feature will be merged someday.

@markkdev
Copy link

Would be awesome to have this feature

@mxdev88
Copy link
Contributor Author

mxdev88 commented Apr 12, 2021

@Digenis do you think this one could make 1.3 alpha release mentioned in #389 also?

@Digenis Digenis added this to the 1.3.0 milestone Apr 12, 2021
@Digenis
Copy link
Member

Digenis commented Apr 12, 2021

I added the 1.3 milestone although I haven't estimated the time to review yet.

I see there is merge conflict but an easy to resolve.

You'll probably want to rebase on master after #398 to get a working CI.

@Digenis
Copy link
Member

Digenis commented Apr 13, 2021

@mxdev88 you can now rebase on master.
The conflicts are only precautions
because of the proximity of the changed lines.

@mxdev88
Copy link
Contributor Author

mxdev88 commented Apr 13, 2021

Hi @Digenis should be ok to merge now. thks

@mxdev88 mxdev88 force-pushed the persist-jobs branch 2 times, most recently from c673838 to 9241bdc Compare April 15, 2021 11:16
@polajenko
Copy link

When can this feature be released?

@mxdev88 mxdev88 mentioned this pull request Nov 20, 2021
@pawelmhm
Copy link
Contributor

pawelmhm commented Nov 22, 2021

This looks really useful, there are unit tests, can you merge recent master branch here and rebuild? Also: finished_to_keep setting seems not documented, worth adding to docs.

edit:

it is in docs already sorry, https://scrapyd.readthedocs.io/en/latest/config.html#finished-to-keep


A class that stores finished jobs. There are 2 implementations provided:

* ``MemoryJobStorage`` (default) jobs are stored and memory and lost when the daemon is restarted
Copy link
Contributor

Choose a reason for hiding this comment

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

default is actually:

scrapyd.jobstorage.MemoryJobStorage

let's put full names here:

scrapyd.jobstorage.MemoryJobStorage
and scrapyd.jobstorage.SqliteJobStorage

@pawelmhm
Copy link
Contributor

pawelmhm commented Nov 22, 2021

I integrated with current master and fixed docs here:#418

@pawelmhm
Copy link
Contributor

related PR merged, commit in master now

@pawelmhm pawelmhm closed this Nov 22, 2021
@pawelmhm pawelmhm mentioned this pull request Nov 23, 2021
@mxdev88 mxdev88 deleted the persist-jobs branch January 7, 2022 15:47
@jpmckinney jpmckinney added pr: replaced for unmerged PRs that were replaced by a PR or commit and removed type: enhancement labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: replaced for unmerged PRs that were replaced by a PR or commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants