[DISCO-3210] [load test: abort] Use async gcs client for manifest #793
Conversation
| return GetManifestResultCode.FAIL, None | ||
| except ValidationError as val_err: | ||
| logger.error(f"Invalid manifest content: {val_err}") | ||
| return GetManifestResultCode.FAIL, None |
There was a problem hiding this comment.
As you can see I've trimmed out a lot of nested logic here but more importantly, I've gotten rid of the SKIP branch.
I'm trying to understand if we really need it? We return the same None data as we do with a FAIL but only the code is different. So logically, I'm seeing it as just another FAIL 🤔
|
|
||
| case GetManifestResultCode.SKIP: | ||
| logger.info("Manifest data was not updated (SKIP).") | ||
| return result_code, None |
There was a problem hiding this comment.
Related to the above comment, we aren't doing much here with SKIP except have a different log.
| @@ -75,12 +75,9 @@ async def _fetch_data(self) -> None: | |||
| } | |||
| self.last_fetch_at = time.time() | |||
|
|
|||
| case GetManifestResultCode.SKIP: | |||
| return None | |||
|
|
|||
There was a problem hiding this comment.
see above two comments!
| self.blob_name = blob_name | ||
| self.bucket = Bucket(storage=self.gcs_client, name=gcs_bucket_path) | ||
|
|
||
| # TODO figure out this | ||
| self.blob_generation = 0 |
There was a problem hiding this comment.
Trying to understand how this helps us? Do we still need it with the async logic? This ties into the SKIP logic for which I've added comments below!
There was a problem hiding this comment.
yeah we should keep the SKIP because when we call get_blob, we can add a clause bucket.get_blob(blob_name, if_generation_not_match=blob_generation) so we're not fetching the same file if the version hasn't changed. it's a slight performance improvement
There was a problem hiding this comment.
the SKIP code exists to catch cases where we didn't fetch a file because we didn't have an updated file to fetch.
| @@ -30,27 +29,22 @@ async def fetch(self) -> tuple[GetManifestResultCode, ManifestData | None]: | |||
| (SKIP, None): If there's no new generation (blob unchanged). | |||
There was a problem hiding this comment.
we should probably update the docstring for this method
There was a problem hiding this comment.
I'm wondering if we even need this method since all it does now is just call fetch_manifest_data. We could just take this out and rename the below method to fetch
References
JIRA: DISCO-3210
Description
PR Review Checklist
Put an
xin the boxes that apply[DISCO-####], and has the same title (if applicable)[load test: (abort|skip|warn)]keywords are applied to the last commit message (if applicable)