rzo1 commented on code in PR #2121: URL: https://github.com/apache/stormcrawler/pull/2121#discussion_r3944167225
########## core/src/main/resources/crawler-default.yaml: ########## @@ -201,8 +201,11 @@ config: # http.content.limit when fetching the robots.txt # (the robots.txt RFC draft requires to fetch and parse at least 500 kiB, # see https://datatracker.ietf.org/doc/html/draft-rep-wg-topic-00#section-2.5) + # A value of -1 means "same as http.content.limit": the robots.txt fetch + # then uses whatever limit is configured for pages, including "no limit" + # when http.content.limit is -1 too. # http.robots.content.limit: 524288 # 512 kiB - http.robots.content.limit: -1 # default same as http.content.limit + http.robots.content.limit: 524288 Review Comment: See the comment on `HttpProtocol.java:451`. With this default, the robots.txt fetch is allowed a larger limit than pages whenever `http.content.limit` is below 512 kiB. That is defensible on RFC grounds but is the opposite of what the new code comment states. ########## core/src/main/java/org/apache/stormcrawler/protocol/okhttp/HttpProtocol.java: ########## @@ -442,7 +442,15 @@ public ProtocolResponse getProtocolOutput(String url, final Metadata metadata) final String pageMaxContentStr = metadata.getFirstValue("http.content.limit"); if (StringUtils.isNotBlank(pageMaxContentStr)) { try { - pageMaxContent = Integer.parseInt(pageMaxContentStr); + int metadataLimit = Integer.parseInt(pageMaxContentStr); + /* + * per-URL metadata can tighten the limit but not remove it: + * a value of -1 means "no limit" and would turn the finite + * global limit of the operator into an unbounded read + */ + if (metadataLimit != -1 || globalMaxContent == -1) { Review Comment: This comment and the new default in `crawler-default.yaml` contradict each other. The comment says per-URL metadata "can tighten the limit but not remove it". The new default `http.robots.content.limit: 524288` **raises** the limit whenever the operator sets a smaller `http.content.limit` (say 64 kiB), because only `-1` is special-cased. Either clamp: ```suggestion if (metadataLimit != -1) { pageMaxContent = globalMaxContent == -1 ? metadataLimit : Math.min(metadataLimit, globalMaxContent); } ``` or keep the widening and reword the comment to say robots.txt is deliberately allowed a larger limit than pages. I prefer the second, since the RFC floor is the reason the key exists, but then the comment has to say that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
