Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize get_artifacts_for_build() with regex filtering #4297

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aditya-wazir
Copy link
Contributor

Introduces regex-based filtering directly in the API to improve performance and reduce number of calls to ab api server.

Introduces regex-based filtering directly in the API to improve performance and reduce number of calls to ab api server.
@svasudevprasad
Copy link
Collaborator

/gcbrun

@@ -118,10 +118,19 @@ def download_artifact(client, bid, target, attempt_id, name, output_directory,
return output_path


def get_artifacts_for_build(client, bid, target, attempt_id='latest'):
def get_artifacts_for_build(client, bid, target, attempt_id='latest',
regexp=''):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default to None please.
Also, please add type annotations to this function.

target=target,
attemptId=attempt_id,
nameRegexp=regexp,
maxResults=100)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is maxResults passed here but not above?

"""Return list of artifacts for a given build."""
request = client.buildartifact().list(
buildId=bid, target=target, attemptId=attempt_id)
if regexp == '':
Copy link
Collaborator

@marktefftech marktefftech Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if not regexp: may read a little better.

Copy link
Collaborator

@marktefftech marktefftech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good.

@aditya-wazir it would be really interesting to see about adding a test for files located in the src/clusterfuzz/_internal/platforms/android/ directory. As far as I can tell, there are not any (at least, not in src/clusterfuzz/_internal/tests/core).

Could you please investigate the level of effort to add a new test file for android here? Maybe we could just start with a single fetch_artifact_test.py file, and build on the structure thereafter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants