[scheduler cleanup phase 1]: Move CacheComparer to pkg/scheduler/inte… #69317

wgliang · 2018-10-02T03:53:44Z

…rnal/cache/comparer

What this PR does / why we need it:

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 #68960

Special notes for your reviewer:
@misterikkit

Release note:

Move CacheComparer to pkg/scheduler/internal/cache/comparer.

shubheksha · 2018-10-02T11:25:13Z

/kind cleanup

misterikkit

I think you need to fix the import in factory.go.

wgliang · 2018-10-07T02:30:30Z

/ping @misterikkit @k82cn @bsalamat

k82cn · 2018-10-07T04:46:52Z

pkg/scheduler/factory/factory.go

Can we add a NewCacheComparer func for this, and add a Run func to cover the following codes, e.g. signal, go routine?

Add a NewCacheComparer is always a good idea. But this PR is just move code. I am more inclined to optimize this part of the code in a new PR. What do you think of it? @k82cn

k82cn · 2018-10-07T04:48:45Z

pkg/scheduler/internal/cache/comparer/cache_comparer.go

Personally, I'd like this to be the first line of struct followed with a empty line to distinguish normal field.

Yes, I agree with you. But maybe we need a new PR for this.

I think it's fine in this PR since this type is moving out of the package that needed to use it. And when a package has basically only one type to export, having a function called simply New is fine.

k82cn · 2018-10-07T04:49:53Z

/approve

LGTM overall, give an approve label firstly :)

k8s-ci-robot · 2018-10-07T04:50:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, wgliang

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [k82cn]

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

misterikkit

Looks pretty good.

misterikkit · 2018-10-08T23:28:55Z

pkg/scheduler/internal/cache/comparer/cache_comparer.go

I think it's fine in this PR since this type is moving out of the package that needed to use it. And when a package has basically only one type to export, having a function called simply New is fine.

misterikkit · 2018-10-08T23:29:13Z

pkg/scheduler/internal/cache/comparer/cache_comparer.go

I don't see where this type is referenced outside of this package. Does it need to be exported?

I suspect this is public because you can't initialize a struct from another package with internal fields.

The comment on this also implies that there is an interface somewhere being satisfied, but I couldn't find one. It's a bit strange that this code was organized this way. Not something that needs to be addressed in this PR though.

I think maybe it should be exported. CacheComparer and its other fields are public.

misterikkit · 2018-10-08T23:30:03Z

pkg/scheduler/internal/cache/comparer/cache_comparer_test.go

Do these tests compile if you change this package name to comparer_test?

It should be ok, but the package name comparer_test should not be the naming method recommended by kubernetes?

em m m, maybe you should say the file name instead of the package name？

I actually did mean the package name. It forces us to test the public API without doing any "mucking" with package internals. It's not always easy, but in this case I think it might be.

wgliang · 2018-10-09T02:36:44Z

/retest

misterikkit

Looks good but I'm holding off on rubber stamping it for one more round of comments.

misterikkit · 2018-10-09T19:17:56Z

pkg/scheduler/internal/cache/comparer/comparer.go

I would prefer this to be named New. Since you brought up the idea of sending follow-up PRs to clean this file, I'm okay with this being changed in a different PR.

misterikkit · 2018-10-09T19:18:52Z

pkg/scheduler/internal/cache/comparer/comparer.go

If you do a follow-up PR. Maybe these methods can be moved onto the CacheComparer type and CompareStrategy can be deleted. I don't see the benefit of having it as a separate, embedded type.

misterikkit · 2018-10-09T19:19:06Z

pkg/scheduler/internal/cache/comparer/comparer_test.go

If it's trivial, please change package name to comparer_test in this PR.

Why do we should name the package as combiner_test? Sorry, I can't understand the benefits of doing this?

But, even if we do, what should we name our file name? If we don't make any changes of files in combiner_test, we will get:
pkg/scheduler/factory/factory.go:63:2: found packages comparer (comparer.go) and comparer (comparer_test.go)

(I'm sure I have set the package name in comparer.go and comparer_test.go as comparer_test. Perhaps contrary to Go's naming rules?)

I think this is coming from my c++ background, where we try to test via public interfaces. For example, suppose one of the structs in our package has a private field protected by a private sync.Mutex. Nothing prevents a unit test from accessing the field without holding the mutex. (Hopefully go test -race would catch it.)

The point is that direct access to private fields might lead us to test the wrong thing. To follow from the previous example, if we have a strong guarantee that all access to the field is protected by a mutex, we can make certain assumptions about concurrent access.

Regardless of the package name, the file name "comparer_test.go" is appropriate.

/lgtm

…rnal/cache/comparer

wgliang · 2018-10-10T14:46:24Z

/retest

wgliang · 2018-10-10T23:11:03Z

/retest

k8s-ci-robot requested review from jayunit100 and k82cn October 2, 2018 03:54

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 2, 2018

wgliang force-pushed the feature/move-cachecompare branch from 88dd293 to af78b3a Compare October 2, 2018 04:01

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 2, 2018

misterikkit suggested changes Oct 3, 2018

View reviewed changes

wgliang force-pushed the feature/move-cachecompare branch from af78b3a to a6381af Compare October 6, 2018 16:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 6, 2018

wgliang force-pushed the feature/move-cachecompare branch 3 times, most recently from dddb5eb to 6dc71a7 Compare October 7, 2018 00:12

k82cn reviewed Oct 7, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2018

misterikkit reviewed Oct 8, 2018

View reviewed changes

wgliang force-pushed the feature/move-cachecompare branch 2 times, most recently from 83de78c to 7a49b39 Compare October 9, 2018 00:12

misterikkit reviewed Oct 9, 2018

View reviewed changes

wgliang force-pushed the feature/move-cachecompare branch from 7a49b39 to 4fbfd02 Compare October 10, 2018 11:24

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2018

wgliang force-pushed the feature/move-cachecompare branch from 4fbfd02 to 48e32af Compare October 10, 2018 11:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2018

wgliang force-pushed the feature/move-cachecompare branch from 48e32af to f63f5ea Compare October 10, 2018 11:46

[scheduler cleanup phase 1]: Move CacheComparer to pkg/scheduler/inte…

f44f55c

…rnal/cache/comparer

wgliang force-pushed the feature/move-cachecompare branch from f63f5ea to f44f55c Compare October 10, 2018 13:28

k8s-ci-robot assigned misterikkit Oct 10, 2018

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

k8s-ci-robot merged commit 256fdb9 into kubernetes:master Oct 11, 2018

[scheduler cleanup phase 1]: Move CacheComparer to pkg/scheduler/inte… #69317

[scheduler cleanup phase 1]: Move CacheComparer to pkg/scheduler/inte… #69317

Conversation

wgliang commented Oct 2, 2018

Uh oh!

shubheksha commented Oct 2, 2018

Uh oh!

misterikkit left a comment

Choose a reason for hiding this comment

Uh oh!

wgliang commented Oct 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k82cn commented Oct 7, 2018

Uh oh!

k8s-ci-robot commented Oct 7, 2018

Uh oh!

misterikkit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgliang Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgliang Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgliang commented Oct 9, 2018

Uh oh!

misterikkit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgliang commented Oct 10, 2018

Uh oh!

wgliang commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wgliang Oct 9, 2018 •

edited

Loading

wgliang Oct 9, 2018 •

edited

Loading