Distinguishing bigquery logging failures severities

[TanuSharma2511]

It resolves #35356

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

[github-actions]

Assigning reviewers:

R: @shunping for label python.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[TanuSharma2511]

Run Python_ML PreCommit 3.9

[github-actions]

Reminder, please take a look at this pr: @shunping

[github-actions]

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @liferoad for label python.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

[github-actions]

Reminder, please take a look at this pr: @liferoad

[github-actions]

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey for label python.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

[shunping]

The logic here is very confusing and error-prone. Maybe we can simplify it?

[shunping]

Let's break down the situation with the help of the following table.

The logging level prior to this PR is as follows. In other words, the logging level is set to ERROR only if should_retry is false and retry_strategy is RETRY_ALWAYS.

retry_backoff	retry_strategy + message	should_retry	log level
not None	RETRY_ALWAYS + any	true	WARN
	RETRY_ON_TRANSIENT_ERROR + transient error	true	WARN
	RETRY_ON_TRANSIENT_ERROR + non-transient error	false	WARN
	RETRY_NEVER + any	false	WARN
None	RETRY_ALWAYS + any	false	ERROR
	RETRY_ON_TRANSIENT_ERROR + transient error	false	WARN
	RETRY_ON_TRANSIENT_ERROR + non-transient error	false	WARN
	RETRY_NEVER + any	false	WARN

The problem pointing out from the issue is that, the user expect that when RETRY_ON_TRANSIENT_ERROR is in use and non-transient errors occur, we should issue a logging message on ERROR level rather than WARNING level.

[shunping]

However, when looking closely to the comment above the code:

        # 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.

It seems there is something we can improve here. I would recommend we do the following changes.

retry_backoff	retry_strategy + message	should_retry	log level
not None	RETRY_ALWAYS + any	true	WARN
	RETRY_ON_TRANSIENT_ERROR + transient error	true	WARN
	RETRY_ON_TRANSIENT_ERROR + non-transient error	false	WARN ERROR
	RETRY_NEVER + any	false	WARN ERROR
None	RETRY_ALWAYS + any	false	ERROR
	RETRY_ON_TRANSIENT_ERROR + transient error	false	WARN ERROR
	RETRY_ON_TRANSIENT_ERROR + non-transient error	false	WARN ERROR
	RETRY_NEVER + any	false	WARN ERROR

Rationales:

• When retry_backoff is None, we will stop retrying. In this case, I think the logging level should be ERROR.

  beam/sdks/python/apache_beam/io/gcp/bigquery.py

  Lines 1758 to 1762 in 01768c3

  	# If retry_backoff is None, then we will not retry and must log.
  	should_retry = any(
  	RetryStrategy.should_retry(
  	self._retry_strategy, entry['errors'][0]['reason'])
  	for entry in errors) and retry_backoff is not None

• When retry_backoff is not None and we continue to retry, we set the logging level to WARN. Two scenarios are in this category: when RETRY_ALL is used or when RETRY_ON_TRANSIENT_ERROR is set and we see a transient error.

---

One thing we can argue here is for retry_strategy=RETRY_NEVER. Since users set it to never retry, we may just use WARN logging level instead of ERROR logging level.

[shunping]

WDYT? @claudevdm @TanuSharma2511

[TanuSharma2511]

In my opinion, for retry_strategy=RETRY_NEVER, there will be only one condition i.e.
RETRY_NEVER + any with should_retry= false, so there is no harm in logging ERROR here. This means whatever error we are not retrying, it is worth highlighting that message as ERROR once.

@claudevdm can give the final lgtm.

[shunping]

In my opinion, for retry_strategy=RETRY_NEVER, there will be only one condition i.e. RETRY_NEVER + any with should_retry= false, so there is no harm in logging ERROR here. This means whatever error we are not retrying, it is worth highlighting that message as ERROR once.

@claudevdm can give the final lgtm.

That makes sense.

I think your current implementation mostly agrees with the table at #35373 (comment) (except for the case in #35373 (comment)).

If so, why don't we simplify it with the following condition?

if (should_retry and self._retry_strategy in 
    [RetryStrategy.RETRY_ON_TRANSIENT_ERROR, RetryStrategy.RETRY_ALL]):
  log_level = logging.WARN
else:
  log_level = logging.ERROR

[shunping]

waiting on author

[TanuSharma2511]

Run Python PreCommit 3.9

[TanuSharma2511]

Run Portable_Python PreCommit 3.9

[shunping]

Thanks for looking into this issue. LGTM now.