Skip to content

Conversation

JRBANCEL
Copy link
Contributor

@JRBANCEL JRBANCEL commented Sep 17, 2024

Purpose of the PR
From an empirical observation, listing the repositories is O(# of OCI objects) and not O(# of repositories). Therefore, what happens when a registry contains repositories with many objects (like hundred of thousands), listing repositories in batch of 100 breaks with an HTTP 502 because the gateway is taking too long list the repositories.

This PR makes the batch size configurable such that these scenarios can be handled. Setting it to a smaller number like 5 just works as the total time spent in the gateway to list fewer repositories is less than the timeout.

I followed the concurrency flag pattern.

@JRBANCEL
Copy link
Contributor Author

@microsoft-github-policy-service agree company="UiPath"

@JRBANCEL
Copy link
Contributor Author

@Wwwsylvia
Copy link
Contributor

Code LGTM. But I'm wondering if we really need this.

listing repositories in batch of 100 breaks with an HTTP 502 because the gateway is taking too long list the repositories.

Getting 502 error when listing repositories sounds like a temporary server-side issue. What do you think @northtyphoon @estebanreyl.

@JRBANCEL
Copy link
Contributor Author

Code LGTM. But I'm wondering if we really need this.

listing repositories in batch of 100 breaks with an HTTP 502 because the gateway is taking too long list the repositories.

Getting 502 error when listing repositories sounds like a temporary server-side issue. What do you think @northtyphoon @estebanreyl.

In our ACR, it is not temporary. It just takes too long and timeouts as it seems proportional to the number of tags in a repository.

@estebanreyl
Copy link
Contributor

This change seems perfectly reasonable to me, I see no reason to not approve it. The reason why repository listing might be slow at times when there are a lot of repos is due to an inefficiency on the server side on how we list repos for which we have a longstanding work item. I won't get too deep into the details here but in essence if there are a lot of repos, we can run into a noticeable delay when querying the repo metadata. It shouldn't be resulting in 502s however (maybe there is some specific network config leading to that, @JRBANCEL are you using a proxy / firewall?). I went over our logs and don't see any 502s returned for our catalog API.

Copy link
Contributor

@estebanreyl estebanreyl left a comment

Choose a reason for hiding this comment

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

LGTM

@JRBANCEL
Copy link
Contributor Author

It shouldn't be resulting in 502s however

To be honest, I can't find that log anymore. But, yes it was some Gateway error regarding a timeout.
We don't use a proxy, this was seen from within an ACR Task.

@JRBANCEL
Copy link
Contributor Author

Also PTAL at #354 when you get a second 🙏

Copy link
Contributor

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

We should also add an example for the new flag in README.md and the command example message.

@JRBANCEL JRBANCEL force-pushed the jrbancel/make-repo-batch-size-configurable branch from e8c6a43 to e426248 Compare September 27, 2024 16:14
@JRBANCEL JRBANCEL force-pushed the jrbancel/make-repo-batch-size-configurable branch from e426248 to f233e4e Compare September 27, 2024 17:24
Copy link
Contributor

@Wwwsylvia Wwwsylvia left a comment

Choose a reason for hiding this comment

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

LGTM

@estebanreyl estebanreyl merged commit 149b295 into Azure:main Oct 11, 2024
6 checks passed
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.

3 participants