rzo1 commented on code in PR #2123:
URL: https://github.com/apache/stormcrawler/pull/2123#discussion_r3944167336
##########
core/src/main/java/org/apache/stormcrawler/persistence/AbstractQueryingSpout.java:
##########
@@ -223,11 +254,19 @@ public void nextTuple() {
timeLastQuerySent = System.currentTimeMillis();
}
+ /** Checks the scheme of a stored URL against the configured {@code
protocols} list. */
+ protected boolean schemeAllowed(String url) {
+ int colon = url.indexOf(':');
+ if (colon <= 0) {
+ return false;
+ }
+ return allowedSchemes.contains(url.substring(0,
colon).toLowerCase(Locale.ROOT));
+ }
+
/**
* Returns the amount of time to wait if the backend was queried too
recently and needs
* throttling or -1 if the backend can be queried straight away.
- */
- private long throttleQueries() {
+ */ private long throttleQueries() {
Review Comment:
A javadoc close and the method signature ended up on one line. CI runs `mvn
-Prat -DskipTests verify -Dskip.format.code=false`, so the format check will
fail on this.
```suggestion
*/
private long throttleQueries() {
```
##########
core/src/main/java/org/apache/stormcrawler/persistence/AbstractQueryingSpout.java:
##########
@@ -116,6 +122,21 @@ public void open(
buffer = URLBuffer.createInstance(stormConf);
+ /*
+ * The store is the crawl instruction set: whatever ends up in it is
+ * fetched. Schemes which are not configured for the crawl must not
+ * re-enter the topology from there, so rows are checked before they
+ * are emitted - URL filtering only runs on the discovery path.
+ */
+ allowedSchemes =
+ ConfUtils.loadListFromConf("protocols", stormConf).stream()
+ .map(String::trim)
+ .map(String::toLowerCase)
Review Comment:
`schemeAllowed` uses `Locale.ROOT`, this does not. On a Turkish-locale
worker, `HTTPS` in `protocols` lowercases to something that never matches and
every URL is rejected.
```suggestion
.map(s -> s.toLowerCase(Locale.ROOT))
```
##########
core/src/main/java/org/apache/stormcrawler/persistence/AbstractQueryingSpout.java:
##########
@@ -201,11 +222,21 @@ public void nextTuple() {
}
List<Object> fields = buffer.next();
String url = fields.get(0).toString();
Review Comment:
`SimpleURLBuffer.next()` can return null while `hasNext()` is true, and
`fields.get(0)` then throws. Pre-existing, but the loop makes it reachable more
often.
```suggestion
List<Object> fields = buffer.next();
if (fields == null) {
break;
}
String url = fields.get(0).toString();
```
##########
core/src/main/java/org/apache/stormcrawler/persistence/AbstractQueryingSpout.java:
##########
@@ -201,11 +222,21 @@ public void nextTuple() {
}
List<Object> fields = buffer.next();
String url = fields.get(0).toString();
+ if (!schemeAllowed(url)) {
+ LOG.warn(
+ "Stored URL {} not fetched: its scheme is not in the
configured list",
+ url);
+ eventCounter.scope("skipped.scheme").incrBy(1);
+ // try the next entry the buffer holds; a rejected row stays in
+ // the store and is skipped again on every query
+ continue;
+ }
this.collector.emit(fields, url);
beingProcessed.put(url, null);
eventCounter.scope("emitted").incrBy(1);
return;
- } else if (timestampEmptyBuffer == -1) {
+ }
+ if (timestampEmptyBuffer == -1 && !buffer.hasNext()) {
Review Comment:
`!buffer.hasNext()` is redundant here; the loop above only exits when it is
already false.
##########
core/src/main/java/org/apache/stormcrawler/persistence/AbstractQueryingSpout.java:
##########
@@ -191,7 +212,7 @@ public void nextTuple() {
timeLastQuerySent = System.currentTimeMillis();
}
- if (buffer.hasNext()) {
+ while (buffer.hasNext()) {
Review Comment:
Changing `if` to `while` plus the `continue` below creates a spin. A store
holding rows the spout will never emit drains the buffer, queries the backend,
gets the same rows back, and drains again. That is exactly the state #2124
leaves behind for anyone with `file:` URLs already persisted.
The comment on line 230 acknowledges it ("a rejected row stays in the store
and is skipped again on every query") but nothing bounds it.
Please emit the rejected URL to the status stream as `Status.ERROR` so the
status updater removes it, instead of only counting it. That also gives the
operator a signal other than one `warn` per row per query.
--
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]
