shunping commented on code in PR #35373:
URL: https://github.com/apache/beam/pull/35373#discussion_r2208283125
##########
sdks/python/apache_beam/io/gcp/bigquery.py:
##########
@@ -1712,9 +1712,15 @@ def _flush_batch(self, destination):
# The log level is:
# - WARNING when we are continuing to retry, and have a deadline.
# - ERROR when we will no longer retry, or MAY retry forever.
- log_level = (
- logging.WARN if should_retry or self._retry_strategy
- != RetryStrategy.RETRY_ALWAYS else logging.ERROR)
+ if (self._retry_strategy == RetryStrategy.RETRY_ON_TRANSIENT_ERROR and
+ not should_retry):
+ log_level = logging.ERROR
+ elif self._retry_strategy == RetryStrategy.RETRY_NEVER:
+ log_level = logging.ERROR
+ elif should_retry or self._retry_strategy !=
RetryStrategy.RETRY_ALWAYS:
Review Comment:
This condition is not accurate:
```python
elif should_retry or self._retry_strategy != RetryStrategy.RETRY_ALWAYS:
log_level = logging.WARN
```
Using the table in
https://github.com/apache/beam/pull/35373#issuecomment-3071605810, we can see
if
(1) `retry_strategy == RetryStrategy.RETRY_ON_TRANSIENT_ERROR`,
(2) the error is a transient error, and
(3) `retry_backoff == None`,
then `should_retry` is false and your current implementation will give a
WARNING message.
The scenario here is that there is a transient error and we exhaust all the
retries. I think we should give an error message at last. Right?
--
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: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
