From 37fb6f6b7b176325ebc8f916d73ed8552092ddcd Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 30 Apr 2019 16:20:53 -0700 Subject: [PATCH] do not drop any `CrawlURI.data` between processing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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... --- .../java/org/archive/modules/CrawlURI.java | 69 +------------------ .../recrawl/FetchHistoryProcessor.java | 1 - .../modules/recrawl/PersistLogProcessor.java | 2 +- .../recrawl/PersistStoreProcessor.java | 2 +- 4 files changed, 3 insertions(+), 71 deletions(-) diff --git a/modules/src/main/java/org/archive/modules/CrawlURI.java b/modules/src/main/java/org/archive/modules/CrawlURI.java index 46a935b2..92d2870f 100644 --- a/modules/src/main/java/org/archive/modules/CrawlURI.java +++ b/modules/src/main/java/org/archive/modules/CrawlURI.java @@ -31,9 +31,6 @@ import static org.archive.modules.CoreAttributeConstants.A_HTTP_RESPONSE_HEADERS import static org.archive.modules.CoreAttributeConstants.A_NONFATAL_ERRORS; import static org.archive.modules.CoreAttributeConstants.A_PREREQUISITE_URI; import static org.archive.modules.CoreAttributeConstants.A_SOURCE_TAG; -import static org.archive.modules.CoreAttributeConstants.A_SUBMIT_DATA; -import static org.archive.modules.CoreAttributeConstants.A_SUBMIT_ENCTYPE; -import static org.archive.modules.CoreAttributeConstants.A_WARC_RESPONSE_HEADERS; import static org.archive.modules.SchedulingConstants.NORMAL; import static org.archive.modules.fetcher.FetchStatusCodes.S_BLOCKED_BY_CUSTOM_PROCESSOR; import static org.archive.modules.fetcher.FetchStatusCodes.S_BLOCKED_BY_USER; @@ -76,7 +73,6 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.logging.Level; import java.util.logging.Logger; @@ -248,15 +244,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { * buggy */ protected long ordinal; - - /** - * Array to hold keys of data members that persist across URI processings. - * Any key mentioned in this list will not be cleared out at the end - * of a pass down the processing chain. - */ - private static final Collection persistentKeys - = new CopyOnWriteArrayList( - new String [] {A_CREDENTIALS_KEY, A_HTTP_AUTH_CHALLENGES, A_SUBMIT_DATA, A_WARC_RESPONSE_HEADERS, A_ANNOTATIONS, A_SUBMIT_ENCTYPE}); /** maximum length for pathFromSeed/hopsPath; longer truncated with leading counter **/ private static final int MAX_HOPS_DISPLAYED = 50; @@ -875,8 +862,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { this.contentLength = UNCALCULATED; // Clear 'links extracted' flag. this.linkExtractorFinished = false; - // Clean the data map of all but registered permanent members. - this.data = getPersistentDataMap(); extraInfo = null; outLinks = null; @@ -886,23 +871,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { // XXX er uh surprised this wasn't here before? fetchType = FetchType.UNKNOWN; } - - public Map getPersistentDataMap() { - if (data == null) { - return null; - } - Map result = new HashMap(getData()); - Set retain = new HashSet(persistentKeys); - - if (containsDataKey(A_HERITABLE_KEYS)) { - @SuppressWarnings("unchecked") - HashSet heritable = (HashSet)getData().get(A_HERITABLE_KEYS); - retain.addAll(heritable); - } - - result.keySet().retainAll(retain); - return result; - } /** * @return Credential avatars. Null if none set. @@ -1132,39 +1100,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { } return (UURI)getData().get(A_HTML_BASE); } - - public static Collection getPersistentDataKeys() { - return persistentKeys; - } - - /** - * Add the key of items you want to persist across - * processings. - * @param s Key to add. - */ - public void addPersistentDataMapKey(String s) { - if (!persistentKeys.contains(s)) { - addDataPersistentMember(s); - } - } - - /** - * Add the key of data map items you want to persist across - * processings. - * @param key Key to add. - */ - public static void addDataPersistentMember(String key) { - persistentKeys.add(key); - } - - /** - * Remove the key from those data map members persisted. - * @param key Key to remove. - * @return True if list contained the element. - */ - public static boolean removeDataPersistentMember(String key) { - return persistentKeys.remove(key); - } private void writeObject(ObjectOutputStream stream) throws IOException { stream.defaultWriteObject(); @@ -1782,9 +1717,7 @@ implements Reporter, Serializable, OverlayContext, Comparable { return containsDataKey(A_FORCE_RETIRE) && (Boolean)getData().get(A_FORCE_RETIRE); } - - - + protected JSONObject extraInfo; public JSONObject getExtraInfo() { diff --git a/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java b/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java index 7bcd5ccf..889fa05c 100644 --- a/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java +++ b/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java @@ -67,7 +67,6 @@ public class FetchHistoryProcessor extends Processor { @Override protected void innerProcess(CrawlURI puri) throws InterruptedException { CrawlURI curi = (CrawlURI) puri; - curi.addPersistentDataMapKey(A_FETCH_HISTORY); HashMap latestFetch = new HashMap(); // save status diff --git a/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java b/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java index d03cb5af..f5dada4d 100644 --- a/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java +++ b/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java @@ -96,7 +96,7 @@ implements Checkpointable, Lifecycle { protected void innerProcess(CrawlURI curi) { log.writeLine(persistKeyFor(curi), " ", new String(Base64.encodeBase64( - SerializationUtils.serialize((Serializable)curi.getPersistentDataMap())))); + SerializationUtils.serialize((Serializable)curi.getData())))); } public void startCheckpoint(Checkpoint checkpointInProgress) {} diff --git a/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java b/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java index 9bb7da92..73468c3b 100644 --- a/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java +++ b/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java @@ -40,7 +40,7 @@ public class PersistStoreProcessor extends PersistOnlineProcessor @Override protected void innerProcess(CrawlURI curi) throws InterruptedException { - store.put(persistKeyFor(curi),curi.getPersistentDataMap()); + store.put(persistKeyFor(curi), curi.getData()); } @Override