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.

Sign up for GitHub

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

Jump to bottom

Replace Parallelize with function ParallelizeUntil and formally depre… #68403

Merged
merged 1 commit into from Oct 6, 2018
Merged

Replace Parallelize with function ParallelizeUntil and formally depre… #68403

merged 1 commit into from Oct 6, 2018

Conversation

wgliang
Copy link
Contributor

@wgliang wgliang commented Sep 7, 2018 •
edited

…cate the Parallelize

What this PR does / why we need it:
And formally deprecate the Parallelize(It's just a variant of ParallelizeUntil).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
This is a follow up PR of #67555
FYI #67555 (comment)

Release note:

Replace Parallelize with function ParallelizeUntil and formally deprecate the Parallelize.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 7, 2018
wgliang
wgliang commented Sep 7, 2018
View reviewed changes
test/e2e/scalability/load.go
scaleclient "k8s.io/client-go/scale"
"k8s.io/client-go/transport"
"k8s.io/client-go/util/workqueue"
"k8s.io/kubernetes/pkg/api/legacyscheme"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, here I sorted out the packages.

@wgliang
Copy link
Contributor Author

wgliang commented Sep 7, 2018

/ping @sttts and @bsalamat

wgliang
wgliang commented Sep 7, 2018
View reviewed changes
staging/src/k8s.io/client-go/util/workqueue/parallelizer.go Outdated
@@ -25,12 +25,6 @@ import (

type DoWorkPieceFunc func(piece int)

// Parallelize is a very simple framework that allows for parallelizing
// N independent pieces of work.
func Parallelize(workers, pieces int, doWorkPiece DoWorkPieceFunc) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delete or just note Deprecate?

Copy link
Contributor

Choose a reason for hiding this comment

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

deprecate. We might have users of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

@roycaihw
Copy link
Member

/assign @yliaog

@yliaog
Copy link
Contributor

yliaog commented Sep 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2018
@wojtek-t
Copy link
Member

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2018
@wgliang
Copy link
Contributor Author

wgliang commented Sep 12, 2018

@wojtek-t
Will we merge in v.1.12?

@wojtek-t
Copy link
Member

Will we merge in v.1.12?

No - it's code-freeze already, and it's not a bug fix or anything like that.

@wgliang
Copy link
Contributor Author

wgliang commented Sep 12, 2018

@wojtek-t
Ok, make sense.

sttts
sttts reviewed Sep 12, 2018
View reviewed changes
pkg/scheduler/algorithm/predicates/metadata.go Outdated
@@ -421,7 +421,7 @@ func getPodsMatchingAffinity(pod *v1.Pod, nodeInfoMap map[string]*schedulercache
appendResult(node.Name, nodeTopologyPairsAffinityPodsMaps, nodeTopologyPairsAntiAffinityPodsMaps)
}
}
workqueue.Parallelize(16, len(allNodeNames), processNode)
workqueue.ParallelizeUntil(nil, 16, len(allNodeNames), processNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

is a nil context an anti-pattern? Never seen that. There is context.Background() and context.TODO().

Copy link
Contributor

Choose a reason for hiding this comment

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

these cases in the scheduler all look like one of those two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, you are right. The default will be context.TODO(), but not nil. But ParallelizeUntil will handle it when the context is nil.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2018
@wgliang
Copy link
Contributor Author

wgliang commented Sep 12, 2018

@sttts I've updated it. But do we need to change the nil in workqueue.Parallelize to the default context.TODO()

kubernetes/staging/src/k8s.io/client-go/util/workqueue/parallelizer.go

Line 31 in 426ef9d

ParallelizeUntil(nil, workers, pieces, doWorkPiece)

@wgliang
Copy link
Contributor Author

wgliang commented Sep 12, 2018

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2018
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2018
@wgliang wgliang force-pushed the master.deprecate-Parallelize branch from e7ecce9 to f10d8c9 Compare October 4, 2018 06:19
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2018
@wgliang
Copy link
Contributor Author

wgliang commented Oct 4, 2018

/ping @sttts
for approve

sttts
sttts reviewed Oct 5, 2018
View reviewed changes
staging/src/k8s.io/client-go/util/workqueue/parallelizer.go Outdated
@@ -25,6 +25,9 @@ import (

type DoWorkPieceFunc func(piece int)

// DEPRECATED: Parallelize is deprecated. ParallelizeUntil is a more
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in the default deprecation style, compare https://rakyll.org/deprecated/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE. Thanks.

@wgliang wgliang force-pushed the master.deprecate-Parallelize branch from f10d8c9 to c2622dd Compare October 5, 2018 09:57
@wgliang
Copy link
Contributor Author

wgliang commented Oct 5, 2018

/retest

yastij
yastij approved these changes Oct 6, 2018
View reviewed changes
Copy link
Member

@yastij yastij left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks @wgliang !

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wgliang, wojtek-t, yastij, yliaog

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

The pull request process is described 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

@wgliang
Copy link
Contributor Author

wgliang commented Oct 6, 2018

/retest

1 similar comment
@wgliang
Copy link
Contributor Author

wgliang commented Oct 6, 2018

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit c00f19b into kubernetes:master Oct 6, 2018
@danielqsj danielqsj mentioned this pull request Apr 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@tao12345666333 tao12345666333 tao12345666333 approved these changes

@yastij yastij yastij approved these changes

@bowei bowei Awaiting requested review from bowei

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@yastij yastij

@yliaog yliaog

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@wgliang @roycaihw @yliaog @wojtek-t @k8s-ci-robot @fejta-bot @sttts @tao12345666333 @yastij