mxtymoshyk commented on issue #28282:
URL: https://github.com/apache/beam/issues/28282#issuecomment-5483211071

   Still relevant, and the code quoted in the description has since moved, so 
here is where it stands.
   
   ## The documentation half is addressed
   
   Both doc gaps @mkuthan listed are fixed in #39938 (open, pending review), 
which rewrites the `withMaxRetryJobs` Javadoc:
   
   > Sets the maximum number of times a failed BigQuery load or copy job is 
retried before the write fails.
   >
   > Only applies when the write method is `FILE_LOADS`. The streaming insert 
and Storage Write API methods retry at the row level and ignore this setting.
   >
   > If this is not called, a bounded (batch) pipeline retries 3 times and an 
unbounded (streaming) pipeline retries 1000 times. Streaming defaults higher 
because failing a bundle in streaming is far more expensive than retrying the 
load job.
   
   That PR also removes the `.setMaxRetryJobs(1000)` default from 
`BigQueryIO.write()` that the description quotes. `getMaxRetryJobs()` is now a 
nullable `Integer` that is unset unless the user calls the setter, which is 
what let #28281 be fixed without raising the bounded default from 3 to 1000. 
The two defaults are now named constants, `BatchLoads.DEFAULT_MAX_RETRY_JOBS` 
(3) and `BatchLoads.DEFAULT_MAX_RETRY_JOBS_UNBOUNDED` (1000).
   
   **The 1000 itself is deliberately untouched by that PR**, because it is this 
issue.
   
   ## What 1000 actually costs
   
   The backoff between retries is configured in `PendingJobManager` 
(`BigQueryHelpers.java:120-128`): 1s initial, exponent 1.5, capped at 60s, 
`withMaxRetries(Integer.MAX_VALUE)` and no `withMaxCumulativeBackoff` override, 
so `FluentBackoff`'s 1000-day cumulative default never binds. `waitForDone` 
(`:161-168`) sleeps once per polling round before re-running each still-pending 
job, and `runJob` gives up when `currentAttempt < maxRetries + 1` fails 
(`:212-221`, `:287-289`).
   
   Nominal sleep sequence: 1s, 1.5s, 2.25s, ... reaching the 60s cap at the 
12th sleep. The first 11 sum to about 171s; the remaining ~988 all sit at the 
cap for 59,280s.
   
   **Total: roughly 16.5 hours** before a permanently failing load job surfaces 
as a bundle failure. `FluentBackoff`'s randomization factor of 0.5 puts the 
real range at roughly 8 to 25 hours. Each of those attempts is a load job that 
reached a terminal `FAILED` state, not a slow one still running, so this is 
genuinely 1000 doomed retries of the invalid-timestamp case in the description.
   
   ## The design question is still open
   
   @ahmedabu98 suggested parsing `jobStatus.getErrorResult()` and failing the 
bundle for non-transient errors, and asked @reuvenlax for better ideas. That 
was three years ago and there was no reply, so there has never been a decision 
to implement against.
   
   The options, as far as I can see:
   
   1. **Classify the error.** Fail fast on non-transient BigQuery errors, keep 
retrying transient ones. Closest to what @ahmedabu98 proposed, and the only one 
that actually answers "no retries for persistent errors". Needs an agreed list 
of which reasons are permanent, and there is currently no test coverage for 
retry behaviour to build on.
   2. **Bound total elapsed time.** Keep the retry count, but stop retrying 
after some wall-clock budget regardless. Additive and it does not shorten the 
retry count anyone currently depends on, but it does not distinguish permanent 
from transient either.
   3. **Lower the constant.** One line, but it silently shortens the retry 
window for every existing streaming pipeline, including ones riding out 
BigQuery quota errors. The code comment says the high limit is intentional, so 
this seems like the least likely to be acceptable.
   
   Worth noting there is currently no test anywhere asserting the unbounded 
default or any retry count that would move, so whichever direction is taken 
would want a regression test alongside it.
   
   I have the retry path paged in from working on #28281 and would be glad to 
implement whichever of these is wanted, but this changes streaming failure 
behaviour, so it seems like a call for the BigQueryIO maintainers rather than 
something to guess at. Is there a preferred direction, or should this go to 
dev@ first? Not claiming the issue in the meantime.
   


-- 
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]

Reply via email to