gtristan commented on code in PR #1645:
URL: https://github.com/apache/buildstream/pull/1645#discussion_r867326780
##########
src/buildstream/_sourcecache.py:
##########
@@ -128,7 +128,7 @@ def pull(self, source):
)
continue
except CASError as e:
- raise SourceCacheError("Failed to pull source {}:
{}".format(display_key, e)) from e
+ raise SourceCacheError("Failed to pull source {}:
{}".format(display_key, e), temporary=True) from e
Review Comment:
This whole `try` block confuses me.
The only functions which might potentially raise an error are
`remote.init()` and `self._pull_source()`.
Neither of these appear to potentially raise `CASError`
* `BaseRemote.init()` clearly cannot raise `CASError`
* `self._pull_source` descends into the following two paths
* `self._store_source()` (indirectly `self._store_proto()`) will just
write the proto directly into the CAS directories using
`utils.save_file_atomic()`
* `AssetRemote.fetch_directory()` will do some grpc-foo and potentially
raise `AssetCacheError`
I think that the `CASError` handling might be leftovers left behind from
previous refactors, it would probably make more sense to:
* Remove the nonsense `CASError` handling
* Handle the `AssetCacheError` in a _more isolated `try` block_ which only
handles exceptions from `AssetRemote.fetch_directory()` (i.e. from within
`self._pull_source()`)
* Raise the temporary failure from `self._pull_source()` in case
`AssetRemote.fetch_directory()` raises `AssetCacheError`
--
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]
