gemini-code-assist[bot] commented on code in PR #39305:
URL: https://github.com/apache/beam/pull/39305#discussion_r3572333800
##########
sdks/python/apache_beam/options/pipeline_options.py:
##########
@@ -1243,18 +1243,27 @@ def _warn_if_soft_delete_policy_enabled(self, arg_name):
key="message")
except ImportError:
_LOGGER.warning('Unable to check soft delete policy due to import
error.')
-
# If either temp or staging location has an issue, we use the valid one for
# both locations. If both are bad we return an error.
def _handle_temp_and_staging_locations(self, validator):
temp_errors = validator.validate_gcs_path(self, 'temp_location')
staging_errors = validator.validate_gcs_path(self, 'staging_location')
+
+ temp_location = getattr(self, 'temp_location', None)
+ staging_location = getattr(self, 'staging_location', None)
+
+ if temp_location is not None and temp_errors:
+ _LOGGER.warning(temp_errors[0])
+
+ if staging_location is not None and staging_errors:
+ _LOGGER.warning(staging_errors[0])
+
if temp_errors and not staging_errors:
- setattr(self, 'temp_location', getattr(self, 'staging_location'))
+ setattr(self, 'temp_location', staging_location)
self._warn_if_soft_delete_policy_enabled('staging_location')
return []
elif staging_errors and not temp_errors:
- setattr(self, 'staging_location', getattr(self, 'temp_location'))
+ setattr(self, 'staging_location', temp_location)
self._warn_if_soft_delete_policy_enabled('temp_location')
return []
Review Comment:
Logging warnings unconditionally for both `temp_location` and
`staging_location` when they are invalid results in redundant log messages when
both locations are invalid (which is a fatal error that will be raised anyway).
Instead, we should only log these warnings when we are actually falling back to
the other location.
```suggestion
if temp_errors and not staging_errors:
if temp_location is not None:
_LOGGER.warning(temp_errors[0])
setattr(self, 'temp_location', staging_location)
self._warn_if_soft_delete_policy_enabled('staging_location')
return []
elif staging_errors and not temp_errors:
if staging_location is not None:
_LOGGER.warning(staging_errors[0])
setattr(self, 'staging_location', temp_location)
self._warn_if_soft_delete_policy_enabled('temp_location')
return []
```
##########
sdks/python/apache_beam/options/pipeline_options_validator.py:
##########
@@ -203,7 +203,12 @@ def validate_gcs_path(self, view, arg_name):
if not self.is_full_string_match(self.GCS_BUCKET, bucket):
return self._validate_error(self.ERR_INVALID_GCS_BUCKET, arg, arg_name)
- if gcs_object is None or '\n' in gcs_object or '\r' in gcs_object:
+ if gcs_object is None:
+ return self._validate_error(
+ "Invalid GCS path: '%s' given for option: %s. "
+ "Did you mean: 'gs://%s/ or gs://some_bucket/%s'?",
+ arg, arg_name, bucket, bucket)
Review Comment:
The suggestion `gs://some_bucket/%s` is confusing because `some_bucket` is a
placeholder name and not the user's actual bucket. If the user provided a
bucket without a trailing slash (e.g., `gs://mybucket`), the most helpful and
direct suggestion is simply to append a trailing slash (e.g.,
`gs://mybucket/`). We should simplify the error message to only suggest the
trailing slash.
```suggestion
if gcs_object is None:
return self._validate_error(
"Invalid GCS path: '%s' given for option: %s. "
"Did you mean: 'gs://%s/'?",
arg, arg_name, bucket)
```
--
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]
