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

[DISCO-3249] Add IconProcessor utility #810

Merged
merged 1 commit into from
Mar 5, 2025
Merged

Conversation

gruberb
Copy link
Member

@gruberb gruberb commented Mar 3, 2025

References

JIRA: DISCO-3249

Description

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format example [DISCO-####], and has the same title (if applicable)
  • [load test: (abort|skip|warn)] keywords are applied to the last commit message (if applicable)
  • Documentation has been updated (if applicable)
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@gruberb gruberb force-pushed the DISCO-3249-image-processor branch 4 times, most recently from 90d9893 to 28ef119 Compare March 3, 2025 21:44
@gruberb gruberb changed the title WIP: Add icon processor util [DISCO-3249] Add IconProcessor utility Mar 3, 2025
@gruberb gruberb requested a review from a team March 3, 2025 21:45
@gruberb gruberb force-pushed the DISCO-3249-image-processor branch 2 times, most recently from dde9083 to 78a6f70 Compare March 4, 2025 13:11
@gruberb gruberb force-pushed the DISCO-3249-image-processor branch 3 times, most recently from 2586471 to 4055e3b Compare March 5, 2025 13:29
@gruberb gruberb requested review from ncloudioj, a team, misaniwere and Herraj and removed request for a team March 5, 2025 13:30
@gruberb gruberb force-pushed the DISCO-3249-image-processor branch from 4055e3b to bf3c55e Compare March 5, 2025 13:33
Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

Just a few more comments, we're almost there.

A few more notes for future iterations:

  • Would this prevent us from running merino/adm locally? Looks like we can use original_url as fallback when exception occurs, so maybe we're good.
  • We need metrics to understand how things perform in the wild.
  • As I pointed out the HTTP connection pool, I realized that we could download images concurrently if necessary. Again, metrics will lead us to the better solution.

@gruberb gruberb force-pushed the DISCO-3249-image-processor branch 3 times, most recently from 6bfdec4 to cdf99cb Compare March 5, 2025 19:11
@gruberb gruberb requested a review from ncloudioj March 5, 2025 19:21
@gruberb
Copy link
Member Author

gruberb commented Mar 5, 2025

We need metrics to understand how things perform in the wild.

I will follow up with metrics in a separate PR.

Copy link
Contributor

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! r+wc.

Please add [load test: abort] to the commit message.

@gruberb gruberb force-pushed the DISCO-3249-image-processor branch 2 times, most recently from 09f1117 to c1340dc Compare March 5, 2025 21:02
@gruberb gruberb changed the title [DISCO-3249] Add IconProcessor utility [DISCO-3249] Add IconProcessor utility [load test: abort] Mar 5, 2025
@gruberb gruberb force-pushed the DISCO-3249-image-processor branch from c1340dc to b5ca1c7 Compare March 5, 2025 21:07
@gruberb gruberb force-pushed the DISCO-3249-image-processor branch from b5ca1c7 to 864a579 Compare March 5, 2025 21:13
@gruberb gruberb changed the title [DISCO-3249] Add IconProcessor utility [load test: abort] [DISCO-3249] Add IconProcessor utility Mar 5, 2025
@gruberb gruberb added this pull request to the merge queue Mar 5, 2025
Merged via the queue into main with commit 60d89be Mar 5, 2025
6 checks passed
@gruberb gruberb deleted the DISCO-3249-image-processor branch March 5, 2025 21:18
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.

2 participants