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

Simplify code around reading from registries #68

Closed

Conversation

justinsb
Copy link
Contributor

@justinsb justinsb commented Jul 9, 2019

Much slower as no longer parallel execution, but more direct/simpler.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 9, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: justinsb
To complete the pull request process, please assign tpepper
You can assign the PR to them by writing /assign @tpepper in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/release Categorizes an issue or PR as relevant to SIG Release. labels Jul 9, 2019
@k8s-ci-robot k8s-ci-robot requested review from dims and listx July 9, 2019 19:31
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 9, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2019
@justinsb
Copy link
Contributor Author

justinsb commented Jul 9, 2019

So this is an experiment to see what the code looks like if it we do it in the more straightforward way (without the threadpool helpers). I personally think the code is much easier to follow, but I wanted to raise it as a topic for discussion @listx .

I think we could re-add concurrent requests within the function and retain most of the simplicity, I think, if we thought it critical.

Much slower as no longer parallel execution, but more direct/simpler.
@listx
Copy link
Contributor

listx commented Jul 9, 2019

This is a non-trivial undertaking --- but I'm not sure why you are trying to change code that is known to work. And as you noted yourself, this only worsens the performance. "If it ain't broken, don't fix it", right?

So from those 2 standpoints, I would give this PR 2 strikes. I would give it a 3rd strike because this breaks the established worker pool pattern we use in other functions that go over the network, such as Promote, GarbageCollect, and ClearRepository.

Are there other reasons for this effort other than what you've already stated?

@listx
Copy link
Contributor

listx commented Jul 9, 2019

Putting in notes here for the record per our discussion offline @justinsb:

The current design, while somewhat unusual compared to other Go code with the workerpool pattern, is solid. It was designed with 2 things in mind: (1) concurrency and (2) testability (with fakes).

Still, it's worth exploring options to make it easier to understand. Let's examine ways to simplify the codebase but still keep these 2 qualities intact.

@dims
Copy link
Member

dims commented Jul 10, 2019

/assign @listx

@listx listx added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019
@k8s-ci-robot
Copy link
Contributor

@justinsb: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2019
@k8s-ci-robot
Copy link
Contributor

@justinsb: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-cip-lint 55dab5c link /test pull-cip-lint
pull-cip-unit-tests 55dab5c link /test pull-cip-unit-tests

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 19, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 19, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants