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

Ensure error and live_status columns are created from the start #191

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

deldesir
Copy link
Collaborator

@deldesir deldesir commented Jun 18, 2024

Makes identification of requested files more robust and logical for

# Abort if there is not a path
if not requested_file:
log.info("No path found in the database")
error = conn.execute("SELECT error, webpath FROM media WHERE error IS NOT NULL").fetchone()
if error:
log.error("[xklb] An error occurred while trying to download %s: %s", error[1], error[0])
self.message = f"{error[1]} failed to download: {error[0]}"
return

@deldesir deldesir self-assigned this Jun 18, 2024
@deldesir deldesir requested a review from holta June 18, 2024 13:07
@deldesir deldesir added bug Something isn't working enhancement New feature or request labels Jun 18, 2024
@holta
Copy link
Member

holta commented Jun 18, 2024

@deldesir
Copy link
Collaborator Author

I can't find the dpaste link that prompted me to propose this. I've seen in one of the logs shared there were a case of missing error column. It was a failed video redownload attempt.

@holta
Copy link
Member

holta commented Jun 18, 2024

  1. What YouTube playlist URL (or any such!) is this tested with?

Clarify smoke test details when you can, Thanks!

@deldesir
Copy link
Collaborator Author

Premature creation of error column is not necessary anymore because videos failed without errors are removed and reprocessed (in next retry) per #193

@deldesir deldesir closed this Jun 18, 2024
@holta
Copy link
Member

holta commented Jun 18, 2024

Premature creation of error column is not necessary anymore because videos failed without errors are removed and reprocessed (in next retry) per #193

Interesting, Ok!

@deldesir
Copy link
Collaborator Author

I reopened this after some reexamination... Error column is expected whenever a download fails.

@holta
Copy link
Member

holta commented Jul 21, 2024

@deldesir deldesir changed the title Ensure error column is created from the start Ensure error and live_status columns are created from the start Jul 21, 2024
@deldesir
Copy link
Collaborator Author

Is it possible the live_status column might also be needed in advance?

Yes, when a playlist URL is submitted.

@holta
Copy link
Member

holta commented Jul 21, 2024

Great news if this is tested + broadly safe, or close?

@deldesir
Copy link
Collaborator Author

Smoke tested on Ubuntu 24.04. Missing columns error doesn't happen anymore

screenshot: N/A

@EMG70
Copy link

EMG70 commented Jul 22, 2024

SUDO IIAB-DIAGNOSTICS: https://dpaste.com/GYCDC4NQG

Tested on a new VM and ran PR#191 on it.
The following video urls were tested .
1.https://www.youtube.com/playlist?list=PLXpbdfI05JvRT07cgyZjwcfSYGiFgnbtM precious plastics
2. https://youtu.be/4BL65HElOPg mondiales( Nzolas's)
3. www.youtube.com/playlist?list=PLqxP5EuGxPncYWWc1cXlcuhfcoo3Wl3Ru The 5 Hardest GCSE Questions Series

RESULTS
-The first video failed to download as seen on task but did appear later in books as a successful download.This happened after triggering download of video 3 above and seems to join both playlist of 29+20 and appeared as downloading 49 videos?.
-The second video downloaded but had a recurring warning during download ( some fragments are taking longer than expected to download,please wait.The whole video eventually downloaded and warnings disappeared from tasks,see screenshot)

-The 3rd video also downloaded Ok as part of combined download albeit randomly (as part of 49 videos url 1 and url 2)
-Precious plastics videos did not create a shelf but split video authors into ( one army and precious plastics) 28/29 videos appear in books as one army and one as precious plastics.Total 29/29 downloaded OK though.This maybe due to owners naming of videos?

Screenshot from 2024-07-22 21-19-06
Screenshot from 2024-07-22 21-32-35
Screenshot from 2024-07-22 22-12-26
Screenshot from 2024-07-22 22-17-18
Screenshot from 2024-07-22 22-18-33

@holta
Copy link
Member

holta commented Jul 22, 2024

@EMG70 I'm not seeing that you ran the git pull https://github.com/deldesir/calibre-web deldesir-patch-44 --no-rebase --no-edit command to install this PR — according to our usual "How do I test a pull request (PR) ?" instructions at: https://github.com/iiab/calibre-web/wiki#how-do-i-test-a-pull-request-pr-

Assuming you're trying to test this PR #191:

The branch deldesir-patch-44 should appear in your git log output, if you're in /usr/local/calibre-web-py3 — and also in your iiab-diagnostics git log output here: https://dpaste.com/GYCDC4NQG#line-1034

@holta
Copy link
Member

holta commented Jul 23, 2024

@EMG70 I apologize:

Ansible was destructively removing all local commits, preventing testing of PR's !

I've now fixed that — so you can try again — testing PR #191 here if you have time:

@EMG70
Copy link

EMG70 commented Jul 23, 2024

Ok will test later today.

@EMG70
Copy link

EMG70 commented Jul 23, 2024

A new VM was created and ran PR#191.
SUDO IIAB-DIAGNOSTICS - https://dpaste.com/FJR5AZQEW

All three video URLs downloaded successfully.
This video ( https://youtu.be/4BL65HElOPg mondiales ( Nzolas's)) of duration 1hr56mins resulted in real time repetitive messages in tasks (some fragments are taking longer than expected to download,please wait). These messages however disappeared upon successful download.Is one message for the whole video good enough or it comes per fragment?see screenshot.

Screenshot from 2024-07-23 13-44-57

Task display after video downloaded successfully.
( https://youtu.be/4BL65HElOPg mondiales ( Nzolas's))
Screenshot from 2024-07-23 13-46-14

@deldesir
Copy link
Collaborator Author

deldesir commented Jul 23, 2024

Smoke tested on Ubuntu 24.04. Missing columns error doesn't happen anymore

Can you test all 3 original problem URL's to confirm... ?

All passed (no errors), except editbooks.py: SAWarning: Object of type not in session, add operation along 'Authors.books' won't proceed #186](#186)

image

Are there any relevant revisions or related clarifications that can now be added at... ?

No, but emphasis should be put on playlists downloads because live_status addition introduced a regression due to missing column

@deldesir
Copy link
Collaborator Author

@EMG70, thanks for testing. The repetitive messages should have been gone.

@holta
Copy link
Member

holta commented Jul 23, 2024

@deldesir I suggest this be merged now, and @codewiz review it quickly for possible improvements after merging, Ok?

@deldesir
Copy link
Collaborator Author

I agree.

@holta
Copy link
Member

holta commented Jul 23, 2024

@codewiz can you review this PR quick — if possible?

(Many Thanks!!)

@holta
Copy link
Member

holta commented Jul 23, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants