abhinav-phi commented on PR #2117: URL: https://github.com/apache/stormcrawler/pull/2117#issuecomment-5574399221
Thank you for the review @rzo1 — all points addressed in 41ea0ba5 (branch rebased onto current main): **Inline 1 — interrupted flush stranded the batch.** Fixed exactly as suggested: the `InterruptedException` handler now removes the batch from `pendingBatches` and calls `failTupleLocally` for each of its URLs, so Storm replays them instead of waiting out the `waitAck` cache expiry. (The comment notes the batch was never handed to the transport, so failing locally loses nothing that reached the frontier.) **Inline 2 — `onBatchError` relied on cache eviction.** Fixed as suggested: the batches still tracked in `pendingBatches` are drained under `batchLock` and their tuples failed immediately. `disableBatchingAndResend` (the UNIMPLEMENTED fallback) already resent its items, so the two paths are now consistent: anything that did not reach the frontier is failed at once, anything that did and was not acked is retried by Storm. **Inline 3 — `timeSpentThrottling`.** The wait is now bracketed with `System.nanoTime()` and the metric is incremented with the time actually spent in `flow.wait` — an ack that wakes the wait early no longer bills the full `urlfrontier.throttling.time.msec`. The 30-second warn threshold uses the measured sum too. **Smaller points:** - `store()` now carries a comment stating that the `tryAcquire` outside the `flow` monitor was considered: a `notifyAll` racing between the failed acquire and the monitor entry only costs one backstop interval, it cannot deadlock. - Lock ordering documented on `sendLock`: code holding `batchLock` may acquire `sendLock`, never the reverse; the same-thread monitor re-entry from `newPutDiscoveredStream` under `batchLock` is noted as safe. - `urlfrontier.batch.size` added to `docs/src/main/asciidoc/configuration.adoc` (URLFrontier table). - Formatting: all four Java files are now formatted with the exact google-java-format version (1.35.0, AOSP) that git-code-format-maven-plugin 6.0 embeds, and `validate-code-format` passes locally with `-Dskip.format.code=false` — this includes the `newSingleThreadScheduledExecutor` lambda block and the two `prepare(...)` lines you flagged, which the earlier local formatter (a different google-java-format build) had left alone. **On the red `rat` job:** it fails only on dependency resolution of `com.github.crawler-commons:urlfrontier-API:2.6`, which is not on Maven Central yet — the url-frontier 2.6 publish workflow succeeded on 2026-08-19 but the deployment appears to be sitting unpublished on the Sonatype Central portal (`central-publishing-maven-plugin` defaults to `autoPublish=false`). I opened crawler-commons/url-frontier#217 reporting this with the evidence; a manual "Publish" of the pending deployment on central.sonatype.com should turn the job green without any change on our side. The code, tests and quality gates (checkstyle, forbiddenapis, format validation, JaCoCo ratios) all pass locally against the 2.6 API and the 2.6/2.5 frontier containers. -- 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]
