WIP: ExtractorYoutubeDL #257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is already a clause for this in logging.properties, but it's using log4j. It was dumping stack traces every time the client was dubious of heritrix's self-signed certificate. Why do we have so many identical log4j.xml's? 🤷♂️
Without this change (or other measures), we sometimes get nulls in the ExtractorYoutubeDL log for containing page information. We'll run this on QA for a while and see if it causes any problems. nlevitt [1:59 PM] https://github.com/internetarchive/heritrix3/blob/master/modules/src/main/java/org/archive/modules/CrawlURI.java#L878 drops some stuff from `CrawlURI.data` after processing a uri, even if it needs to be processed again there is a list of keys that shouldn’t be dropped (`persistentKeys`), but it is final and private so if you’re writing your own heritrix module and you want to keep some information in CrawlURI.data, it usually works, except when the url is processed more than once (like when it needs a prereq like robots.txt the first time) in practice it seems that most data is persisted, that is, most commonly used keys are in `persistentKeys` in a crawl with pretty standard configuration i’m mostly seeing `prerequisite-uri` dropped and occasionally `fetch-completed-time` and `fetch-began-time` being dropped i’m highly skeptical of the value of dropping keys at all and i’m tempted to get rid of this entirely, make all the keys persistent in other words soliciting feedback (edited) anjackson [2:39 PM] My immediate reaction is HARD AGREE. It looks like Really Old Code though (https://github.com/internetarchive/heritrix3/blame/7d3eff5269142c77fa4b988396153f4c29d16caa/modules/src/main/java/org/archive/modules/CrawlURI.java#L878) so the reasons for doing so may have been lost in time. Hm, looking at usage: https://github.com/internetarchive/heritrix3/blob/a60b2ef3875ad47f57b0c6b3c0b19f86c40a12f7/engine/src/main/java/org/archive/crawler/frontier/WorkQueueFrontier.java#L954-L955 engine/src/main/java/org/archive/crawler/frontier/WorkQueueFrontier.java:954-955 curi.processingCleanup(); // lose state that shouldn't burden // retry I guess there's a concern that there may be state in there that is set during a fetch and may cause problems if the same CrawlURI is deferred? But I'm not aware of anything in the fetch chain that behaves like that. nlevitt [3:02 PM] oh, i missed `CrawlURI.addDataPersistentMember(String)` et al. still...
ato
reviewed
May 1, 2019
contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java
Outdated
Show resolved
Hide resolved
... to avoid hanging see https://github.com/internetarchive/heritrix3/pull/257/files#r279990349 thanks Alex!
reading stderr in a separate thread doesn't help if you wait for that thread to finish before reading stdout
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.