## Summary I have an element and I have some custom source plugin I've developed, and their `fetch()` operation is costly. I noticed that, if one source in the element changes, FetchQueue asks all sources to be fetched again including the ones that reported themselves `Consistency.CACHED`. - From user perspective, I don't think this should happen, it is a waste of time. - From plugin developer perspective, I don't want to include `handle repeated calls to fetch()` logic in my plugin's `fetch()`, instead I want to keep it only in `get_consistency()`. Additionally, for example, we are hitting a line we should never hit in DownloadableFileSource-based plugins: https://gitlab.com/BuildStream/buildstream/blob/a53d6d1f81913d639175cbd503d9d71ec7133a4e/buildstream/plugins/sources/_downloadablefilesource.py#L132 I was also able to observe this in `git.py` plugin. `git` and `tar` for example can handle this at the moment but this is still wrong. ## Steps to reproduce I used a patch like this to add some print statements - you can add your own.. ```diff diff --git a/buildstream/plugins/sources/_downloadablefilesource.py b/buildstream/plugins/sources/_downloadablefilesource.py index f5c5b3d0..80099787 100644 --- a/buildstream/plugins/sources/_downloadablefilesource.py +++ b/buildstream/plugins/sources/_downloadablefilesource.py @@ -132,6 +132,7 @@ class DownloadableFileSource(Source): # not be called if the source is already Consistency.CACHED. # if os.path.isfile(self._get_mirror_file()): + self.info("defensive check hit?? in {}".format(self.name)) return # pragma: nocover # Download the file, raise hell if the sha256sums don't match, @@ -162,6 +163,7 @@ class DownloadableFileSource(Source): etagfile.write(etag) def _ensure_mirror(self): + self.info("_ensure_mirror in {}".format(self.name)) # Downloads from the url and caches it according to its sha256sum. try: with self.tempdir() as td: diff --git a/buildstream/source.py b/buildstream/source.py index d6681b96..f6e3c07a 100644 --- a/buildstream/source.py +++ b/buildstream/source.py @@ -973,6 +973,7 @@ class Source(Plugin): # Tries to call fetch for every mirror, stopping once it succeeds def __do_fetch(self, **kwargs): + self.info("__do_fetch called for {}".format(self.name)) project = self._get_project() context = self._get_context() @@ -1028,6 +1029,7 @@ class Source(Plugin): else: mirrors = project.config.mirrors if not mirrors or not alias: + self.info("{} calling fetch()!".format(self.name)) self.fetch(**kwargs) return ``` Prepare a simple bst file with one `tar` source. `bst fetch` it. Later add one more `tar` source. Tar is smart and its `fetch()` terminates early, but still a problem. Do the same for `git` sources. ## What is the current bug behavior? One non-cached source is enough to trigger `fetch()` on all sources. ## What is the expected correct behavior? Buildstream should just fetch the sources that are not already cached. ## Relevant logs and/or screenshots [//]: # (Paste any relevant logs - please use code blocks ``` to format console output, logs, and code as it's hard to read otherwise. You can also add a Snippet and link it here. Check the markdown giude on Gitlab for further tips) ## Possible fixes Here is [the line](https://gitlab.com/BuildStream/buildstream/blob/a53d6d1f81913d639175cbd503d9d71ec7133a4e/buildstream/_scheduler/queues/fetchqueue.py#L45) we need to add a check, I believe. We might need to discuss if it'd be better to keep that logic in `Element` instead because there would be a `source.get_consistency() == Consistency.CACHED` check - too many details for a `Queue` perhaps. ## Other relevant information * BuildStream version affected: 1.3.0+1017.g761e7570. ----