Re: [PR] [client] Throw Exception when retry several times when downloading log. [fluss]

["Jackeyzhe (via GitHub)" <gi...@apache.org>]

Jackeyzhe commented on PR #1752:
URL: https://github.com/apache/fluss/pull/1752#issuecomment-5496163004

   ## Review: PR #1752 — remote-log download retry / exception propagation
   
   Thanks for picking this up — surfacing the download error to the consumer instead of retrying
   forever is the right call, and the `LogFetchBuffer` error-propagation design (`peek()`/`poll()`
   throw `FetchException`) is clean. I left one correctness concern and a couple of nits.
   
   ### 🔴 [must-fix] Semaphore permit leak on permanent download failure
   
   The permit accounting changed in a way that can **stall the whole remote-log downloader** after
   `CLIENT_SCANNER_REMOTE_LOG_PREFETCH_NUM` permanently-failing segments.
   
   Background: `prefetchSemaphore` is released only in two places — (a) the `closed`/`early-return`
   branches, and (b) `RemoteCompletedFetch.drain()` → `recycleCallback.run()` → `recycleRemoteLog()`
   (line 140). i.e. the permit is returned when the **consumer recycles the successfully-downloaded
   file**, not when the download finishes. That matches the javadoc "blocks until there is capacity
   (the fetched file is consumed)".
   
   On the **failure** path this breaks:
   - `main` released the permit immediately on every failure (`prefetchSemaphore.release(); segmentsToFetch.add(request);`) — see the removed lines in the diff.
   - The new `handleFetchException(...)` does **not** release the permit. On exhaustion it only calls `request.future.completeExceptionally(...)`.
   - But the exceptional `RemoteCompletedFetch` is never drained: `LogFetchBuffer.poll()` calls `checkException()` and throws `FetchException` *before* the consumer reaches `drain()`, so `recycleCallback` is never run → the permit is never returned.
   
   Net effect: each segment whose remote log is permanently unreadable (exactly the scenario this PR
   wants to surface) leaks one permit. After `PREFETCH_NUM` such segments, `fetchOnce()` blocks
   forever at `prefetchSemaphore.acquire()` and the scanner's remote-log prefetch halts.
   
   Suggested fix — release the permit on the **exhaustion** branch (mirror `main`'s immediate
   release), but keep holding it while retries are still in flight:
   
   ```java
   private void handleFetchException(
           RemoteLogDownloadRequest request, Throwable throwable, int retryCount, long startTime) {
       LOG.error(...);
       scannerMetricGroup.remoteFetchErrorCount().inc();
       if (retryCount >= 1) {
           downloadRemoteLog(request, retryCount - 1, startTime);   // permit still held until success-recycle
       } else {
           prefetchSemaphore.release();                            // <-- return the permit; no file to recycle
           request.future.completeExceptionally(
                   new IOException(
                           String.format("Failed to download remote log segment file %s, retry count %d",
                                   request.getFsPathAndFileName().getFileName(), MAX_RETRY_COUNT),
                           throwable));
       }
   }
   ```
   
   Worth adding a test that asserts the semaphore count returns to full after N permanent failures
   (the current `RemoteLogDownloaderTest` manually calls `getRecycleCallback().run()`, which papers
   over this path).
   
   ### 🟡 [nit] No backoff between retries
   
   `handleFetchException` re-issues the download synchronously (chained through the future callback)
   with zero delay. For a transient error that needs the remote FS to recover, 5 tight retries may
   all fail in microseconds. Consider a small backoff (or reuse the existing retry config) so retries
   actually help with transient blips. Not blocking.
   
   ### 🟡 [nit] `MAX_RETRY_COUNT = 5` is a hard-coded magic constant
   
   For a 1.0 stability knob it'd be nicer to expose it as a config option (with a sensible default),
   so operators can tune it without recompiling. Optional.
   
   ### ✅ Tests
   
   `LogFetcherTest` (+95) and `RemoteLogDownloaderTest` (+47) cover the new behavior well. The only
   gap is the permit-leak path above — please extend the test as suggested.
   
   Overall this is a good direction; the permit fix is the one thing I'd want resolved before merge.
   


-- 
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: issues-unsubscribe@fluss.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org