pickfirst: Move var for mocking the shuffle func from internal/internal to pickfirst/internal #7698

arjan-bal · 2024-10-03T09:00:25Z

For someone reading the pickfirst code, it can be a little misleading as to why pickfirst is getting the shuffle func from a variable with a name having the ForTesting suffix. This PR provides a setter in the internal package that can be used to replace the shuffler being used as suggested in #7515.

RELEASE NOTES: N/A

…esting

codecov · 2024-10-03T09:03:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.77%. Comparing base (5f178a8) to head (4646f45).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7698      +/-   ##
==========================================
- Coverage   81.77%   81.77%   -0.01%     
==========================================
  Files         361      361              
  Lines       27826    27825       -1     
==========================================
- Hits        22756    22755       -1     
+ Misses       3865     3863       -2     
- Partials     1205     1207       +2

Files with missing lines	Coverage Δ
balancer/pickfirst/pickfirst.go	`84.42% <100.00%> (+0.68%)`	⬆️

... and 13 files with indirect coverage changes

test/pickfirst_test.go

internal/internal.go

easwars · 2024-10-03T19:06:29Z

Also, the PR title has two setters.

dfawley · 2024-10-03T20:40:30Z

Also, the PR title has two setters.

And there is a typo in reference. We should feel free to edit each other's PR titles to help.

dfawley · 2024-10-03T20:43:01Z

balancer/pickfirst/pickfirst.go

-var logger = grpclog.Component("pick-first-lb")
+var (
+	logger      = grpclog.Component("pick-first-lb")
+	shuffleFunc = rand.Shuffle


Nit: I like to name these variables packagenameFunctionName. So this would be randShuffle. E.g.:

grpc-go/internal/wrr/random.go

Line 49 in 67e47fc

var randInt63n = rand.Int63n

dfawley · 2024-10-03T20:44:18Z

balancer/pickfirst/pickfirst.go


 const (
 	// Name is the name of the pick_first balancer.
 	Name      = "pick_first"
 	logPrefix = "[pick-first-lb %p] "
 )

+func init() {
+	balancer.Register(pickfirstBuilder{})
+	internal.ShuffleAddressListForTesting = func(sf func(n int, swap func(i, j int))) func(n int, swap func(i, j int)) {


How about SetPickFirstRandShuffleForTesting?

Renamed to SetRandShuffleForTesting after moving the variable into pickfirst/internal omitting PickFirst because it's now in the pickfirst directory.

internal/internal.go

dfawley · 2024-10-03T20:46:25Z

Also, thank you for fixing this. It was just bothering me the other day.

easwars · 2024-10-07T18:45:57Z

balancer/pickfirst/internal/internal.go

+// Package internal contains gRPC-internal code, to avoid polluting
+// the godoc of the top-level grpc package.


Maybe something like?

// Package internal contains code internal to the pickfirst package.

Updated as suggested.

easwars · 2024-10-07T18:46:40Z

balancer/pickfirst/internal/internal.go

+var (
+	// SetRandShuffleForTesting pseudo-randomizes the order of addresses. n
+	// is the number of elements.  swap swaps the elements with indexes i and j.
+	// It returns the shuffle func before it was replaced.


This last line in the comment is not true anymore.

dfawley · 2024-10-07T20:22:54Z

balancer/pickfirst/internal/internal.go

+	SetRandShuffleForTesting func(sf func(n int, swap func(i, j int)))
+
+	// RevertRandShuffleFunc sets the real shuffle function back after testing.
+	RevertRandShuffleFunc func()


Nit: RevertRandShuffleForTesting to be consistent.

dfawley · 2024-10-07T20:24:51Z

balancer/pickfirst/internal/internal.go

+
+	// RevertRandShuffleFunc sets the real shuffle function back after testing.
+	RevertRandShuffleFunc func()
+)


Alternate proposal:

package internal var RandShuffle = rand.Shuffle package pickfirst // when it's time to shuffle, call internal.RandShuffle package pickfirst_test // set internal.RandShuffle directly

I think this would be a bit simpler for everyone to use.

This seems to be the same as the existing situation (i.e. without the changes in this PR), the only difference being the removal of the suffix ForTesting from the shuffle function's name (internal.RandShuffle now).

@easwars what do you think about this?

I'm ok with this too, but I'd like the var RandShuffle to be in pickfirst/internal instead of the top-level internal package. We already have too much stuff in the top-level internal package and it would be good to move things out wherever possible.

Changed as suggested.

but I'd like the var RandShuffle to be in pickfirst/internal instead of the top-level internal package

Yes, this was what I had in mind, too, just didn't explain it correctly.

dfawley

Thanks!

…al to pickfirst/internal (grpc#7698)

Provide shuffe func setter instead of direct access to shuffler for t…

66068ae

…esting

arjan-bal added the Type: Internal Cleanup Refactors, etc label Oct 3, 2024

arjan-bal added this to the 1.68 Release milestone Oct 3, 2024

arjan-bal requested a review from easwars October 3, 2024 09:00

arjan-bal assigned easwars Oct 3, 2024

easwars reviewed Oct 3, 2024

View reviewed changes

test/pickfirst_test.go Outdated Show resolved Hide resolved

internal/internal.go Outdated Show resolved Hide resolved

easwars assigned arjan-bal and unassigned easwars Oct 3, 2024

dfawley changed the title ~~pickfirst: Provide a setter setter for the shuffle func instead of a direct refference~~ pickfirst: Provide a setter for the shuffle func instead of a direct reference Oct 3, 2024

dfawley reviewed Oct 3, 2024

View reviewed changes

Move pickfirst tests from /test to /balancer/pickfirst

cc6a708

arjan-bal force-pushed the pickfirst-cleanup branch from 5850e5c to cc6a708 Compare October 4, 2024 18:11

arjan-bal assigned easwars and unassigned arjan-bal Oct 4, 2024

easwars approved these changes Oct 7, 2024

View reviewed changes

easwars assigned arjan-bal and unassigned easwars Oct 7, 2024

dfawley reviewed Oct 7, 2024

View reviewed changes

address review comments

6e0a2d0

arjan-bal assigned easwars and arjan-bal and unassigned arjan-bal and easwars Oct 8, 2024

Swap out var directly instead of using setters

1ea5a3c

arjan-bal force-pushed the pickfirst-cleanup branch from e826fd8 to 1ea5a3c Compare October 8, 2024 17:22

arjan-bal assigned easwars Oct 8, 2024

arjan-bal assigned dfawley and unassigned arjan-bal Oct 8, 2024

arjan-bal requested review from easwars and dfawley October 8, 2024 17:23

Merge branch 'master' of github.com:grpc/grpc-go into pickfirst-cleanup

4646f45

arjan-bal changed the title ~~pickfirst: Provide a setter for the shuffle func instead of a direct reference~~ pickfirst: Move var for mocking the shuffle func from internal/internal to pickfirst/internal Oct 8, 2024

dfawley approved these changes Oct 8, 2024

View reviewed changes

dfawley assigned arjan-bal and easwars and unassigned easwars and dfawley Oct 8, 2024

arjan-bal unassigned easwars Oct 9, 2024

arjan-bal merged commit b8ee37d into grpc:master Oct 9, 2024
14 checks passed

arjan-bal deleted the pickfirst-cleanup branch October 9, 2024 09:39

janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Oct 15, 2024

pickfirst: Move var for mocking the shuffle func from internal/intern…

83158ca

…al to pickfirst/internal (grpc#7698)

tbg mentioned this pull request Nov 27, 2024

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pickfirst: Move var for mocking the shuffle func from internal/internal to pickfirst/internal #7698

pickfirst: Move var for mocking the shuffle func from internal/internal to pickfirst/internal #7698

arjan-bal commented Oct 3, 2024

codecov bot commented Oct 3, 2024 •

edited

Loading

easwars commented Oct 3, 2024

dfawley commented Oct 3, 2024

dfawley Oct 3, 2024

arjan-bal Oct 4, 2024

dfawley Oct 3, 2024

arjan-bal Oct 4, 2024

dfawley commented Oct 3, 2024

easwars Oct 7, 2024

arjan-bal Oct 8, 2024

easwars Oct 7, 2024

arjan-bal Oct 8, 2024

dfawley Oct 7, 2024

arjan-bal Oct 8, 2024

dfawley Oct 7, 2024

arjan-bal Oct 8, 2024

easwars Oct 8, 2024

arjan-bal Oct 8, 2024

dfawley Oct 8, 2024

dfawley left a comment

		// Package internal contains gRPC-internal code, to avoid polluting
		// the godoc of the top-level grpc package.

pickfirst: Move var for mocking the shuffle func from internal/internal to pickfirst/internal #7698

pickfirst: Move var for mocking the shuffle func from internal/internal to pickfirst/internal #7698

Conversation

arjan-bal commented Oct 3, 2024

codecov bot commented Oct 3, 2024 • edited Loading

Codecov Report

easwars commented Oct 3, 2024

dfawley commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 3, 2024 •

edited

Loading