stickiness: add stickiness support #1969

menghanl · 2018-04-05T19:34:41Z

This change is

MakMukhi · 2018-04-10T17:22:01Z

clientconn.go

@@ -1394,6 +1403,12 @@ func (ac *addrConn) getState() connectivity.State {
 	return ac.state
 }

+func (ac *addrConn) getCurAddr() resolver.Address {
+	ac.mu.Lock()
+	defer ac.mu.Unlock()


Seems like this will be called on the hot path. How about we don't use defer, if that's the case?

MakMukhi · 2018-04-10T17:25:26Z

picker_wrapper.go

@@ -60,6 +70,19 @@ func (bp *pickerWrapper) connectionError() error {
 	return err
 }

+func (bp *pickerWrapper) updateStickinessMDKey(newKey string) {
+	bp.stickinessMu.Lock()
+	if bp.stickinessMDKey != newKey {


Can stickinessMDKey be updated on the fly? Currently, it seems like this method is called only during initialization.

Yes, it's updated as part of the service config.

MakMukhi · 2018-04-10T17:33:08Z

picker_wrapper.go

@@ -40,10 +42,18 @@ type pickerWrapper struct {
 	// The latest connection happened.
 	connErrMu sync.Mutex
 	connErr   error
+
+	stickinessMu    sync.Mutex
+	stickinessMDKey string


If this can change after initialization, how about using atomic.Value instead of mutex?
Since stickyStore has a mutex inside of it, we don't need another mutex to protect it.

MakMukhi · 2018-04-10T17:34:52Z

Documentation/stickiness.md

+
+With load balancer, each RPC pick a different backend based on the load
+balancing policy. Stickiness policies try to preserve peers for the duration of
+a session.


Explain this a little more?

MakMukhi · 2018-04-10T17:45:20Z

picker_wrapper.go

+
+type stickyStore struct {
+	mu    sync.Mutex
+	store map[string]*stickyStoreEntry


Can this key not be a string? This key doesn't depend on service config right?

Also, How about instead of using a map we use 10 slices each with it's own mutex and convert the string into an int (without using something as expensive as hashing), and mod the int with 10 and use that slice for it?
One possible way of string to int could be:

a := "some_string" num := 0 for _, b := range []byte(a){ num += int(b) }

The key comes from metadata, it has to be string. No hashing should be applied to the stickiness key.

menghanl

Thanks for the review!
All fixed. PTAL.

menghanl · 2018-04-10T21:34:49Z

picker_wrapper.go

@@ -60,6 +70,19 @@ func (bp *pickerWrapper) connectionError() error {
 	return err
 }

+func (bp *pickerWrapper) updateStickinessMDKey(newKey string) {
+	bp.stickinessMu.Lock()
+	if bp.stickinessMDKey != newKey {


Yes, it's updated as part of the service config.

menghanl · 2018-04-10T23:00:33Z

clientconn.go

@@ -1394,6 +1403,12 @@ func (ac *addrConn) getState() connectivity.State {
 	return ac.state
 }

+func (ac *addrConn) getCurAddr() resolver.Address {
+	ac.mu.Lock()
+	defer ac.mu.Unlock()


menghanl · 2018-04-10T23:02:22Z

picker_wrapper.go

+
+type stickyStore struct {
+	mu    sync.Mutex
+	store map[string]*stickyStoreEntry


The key comes from metadata, it has to be string. No hashing should be applied to the stickiness key.

menghanl · 2018-04-10T23:05:23Z

Documentation/stickiness.md

+
+With load balancer, each RPC pick a different backend based on the load
+balancing policy. Stickiness policies try to preserve peers for the duration of
+a session.


menghanl · 2018-04-10T23:15:42Z

picker_wrapper.go

@@ -40,10 +42,18 @@ type pickerWrapper struct {
 	// The latest connection happened.
 	connErrMu sync.Mutex
 	connErr   error
+
+	stickinessMu    sync.Mutex
+	stickinessMDKey string


menghanl · 2018-04-18T17:49:24Z

Benchmark result for non-sticky RPCs:

==== before ====
 
go run benchmark/benchmain/main.go -benchtime=10s -workloads=all -compression=on -maxConcurrentCalls=1000 -trace=off -reqSizeBytes=1 -respSizeBytes=1 -networkMode=Local
Unary-traceMode_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1000-reqSize_1B-respSize_1B-Compressor_true: 
50_Latency: 14071.5150 µs 	90_Latency: 16441.2350 µs 	99_Latency: 20031.4030 µs 	Avg latency: 14210.7170 µs 	Count: 703661 	13866 Bytes/op	173 Allocs/op	
Histogram (unit: µs)
Count: 703661  Min: 613.9  Max: 45838.0  Avg: 14210.72
------------------------------------------------------------
[     613.868000,      613.869000)       1    0.0%    0.0%  
[     613.869000,      613.875089)       0    0.0%    0.0%  
[     613.875089,      613.918257)       0    0.0%    0.0%  
[     613.918257,      614.224279)       0    0.0%    0.0%  
[     614.224279,      616.393728)       0    0.0%    0.0%  
[     616.393728,      631.773371)       0    0.0%    0.0%  
[     631.773371,      740.802589)       0    0.0%    0.0%  
[     740.802589,     1513.731516)       0    0.0%    0.0%  
[    1513.731516,     6993.172135)       1    0.0%    0.0%  
[    6993.172135,    45837.972000)  703658  100.0%  100.0%  ##########
[   45837.972000,             inf)       1    0.0%  100.0%  
 
Stream-traceMode_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1000-reqSize_1B-respSize_1B-Compressor_true: 
50_Latency: 16236.1490 µs 	90_Latency: 49637.4370 µs 	99_Latency: 76499.9850 µs 	Avg latency: 23501.6690 µs 	Count: 425264 	41246 Bytes/op	40 Allocs/op	
Histogram (unit: µs)
Count: 425264  Min: 215.3  Max: 131301.7  Avg: 23501.67
------------------------------------------------------------
[      215.284000,       215.285000)       1    0.0%    0.0%  
[      215.285000,       215.291979)       0    0.0%    0.0%  
[      215.291979,       215.347665)       0    0.0%    0.0%  
[      215.347665,       215.791987)       0    0.0%    0.0%  
[      215.791987,       219.337250)       0    0.0%    0.0%  
[      219.337250,       247.625060)       0    0.0%    0.0%  
[      247.625060,       473.334737)       6    0.0%    0.0%  
[      473.334737,      2274.282144)      53    0.0%    0.0%  
[     2274.282144,     16644.120436)  221560   52.1%   52.1%  #####
[    16644.120436,    131301.690000)  203643   47.9%  100.0%  #####
[   131301.690000,              inf)       1    0.0%  100.0%  
 
==== after ====

go run benchmark/benchmain/main.go -benchtime=10s -workloads=all -compression=on -maxConcurrentCalls=1000 -trace=off -reqSizeBytes=1 -respSizeBytes=1 -networkMode=Local
Unary-traceMode_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1000-reqSize_1B-respSize_1B-Compressor_true: 
50_Latency: 13.8357 ms 	90_Latency: 15.9277 ms 	99_Latency: 19.8321 ms 	Avg latency: 13.9379 ms 	Count: 717441 13868 Bytes/op	174 Allocs/op	
Histogram (unit: ms)
Count: 717441  Min:   3.8  Max:  33.6  Avg: 13.94
------------------------------------------------------------
[       3.845331,        3.845332)       1    0.0%    0.0%  
[       3.845332,        3.845338)       0    0.0%    0.0%  
[       3.845338,        3.845377)       0    0.0%    0.0%  
[       3.845377,        3.845641)       0    0.0%    0.0%  
[       3.845641,        3.847429)       0    0.0%    0.0%  
[       3.847429,        3.859533)       0    0.0%    0.0%  
[       3.859533,        3.941452)       0    0.0%    0.0%  
[       3.941452,        4.495892)       0    0.0%    0.0%  
[       4.495892,        8.248425)     553    0.1%    0.1%  
[       8.248425,       33.646118)  716886   99.9%  100.0%  ##########
[      33.646118,             inf)       1    0.0%  100.0%  
 
Stream-traceMode_false-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1000-reqSize_1B-respSize_1B-Compressor_true: 
50_Latency: 16425.5590 µs 	90_Latency: 48938.5340 µs 	99_Latency: 75172.7280 µs 	Avg latency: 23827.6740 µs 	Count: 419785 	42069 Bytes/op	40 Allocs/op	
Histogram (unit: µs)
Count: 419785  Min: 184.6  Max: 141370.9  Avg: 23827.67
------------------------------------------------------------
[      184.603000,       184.604000)       1    0.0%    0.0%  
[      184.604000,       184.611045)       0    0.0%    0.0%  
[      184.611045,       184.667724)       0    0.0%    0.0%  
[      184.667724,       185.123712)       0    0.0%    0.0%  
[      185.123712,       188.792190)       0    0.0%    0.0%  
[      188.792190,       218.305535)       2    0.0%    0.0%  
[      218.305535,       455.743926)       9    0.0%    0.0%  
[      455.743926,      2365.964217)      39    0.0%    0.0%  
[     2365.964217,     17733.915193)  232117   55.3%   55.3%  ######
[    17733.915193,    141370.921000)  187616   44.7%  100.0%  ####
[   141370.921000,              inf)       1    0.0%  100.0%

MakMukhi · 2018-04-18T22:10:22Z

picker_wrapper.go, line 73 at r4 (raw file):

func (bp *pickerWrapper) updateStickinessMDKey(newKey string) {
	if oldKey, _ := bp.stickinessMDKey.Load().(string); oldKey != newKey {

How about atomic.Swap instead of atomic.Load ?

Comments from Reviewable

MakMukhi · 2018-04-18T22:10:27Z

picker_wrapper.go, line 80 at r4 (raw file):

func (bp *pickerWrapper) clearStickinessState() {
	if oldKey, _ := bp.stickinessMDKey.Load().(string); oldKey != "" {

Same as above

Comments from Reviewable

MakMukhi · 2018-04-18T22:10:36Z

stickiness_test.go, line 172 at r4 (raw file):

	}

	if picked[0] != 0 && picked[1] != 0 {

How about: (picked[0] != 0 && picked[1] == 0) || (picked[0] == 0 && picked[1] !=0) ?

Comments from Reviewable

menghanl · 2018-04-18T22:31:35Z

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.

picker_wrapper.go, line 73 at r4 (raw file):

Previously, MakMukhi (mmukhi) wrote…

How about atomic.Swap instead of atomic.Load ?

Done.

picker_wrapper.go, line 80 at r4 (raw file):

Previously, MakMukhi (mmukhi) wrote…

Same as above

Done.

stickiness_test.go, line 172 at r4 (raw file):

Previously, MakMukhi (mmukhi) wrote…

How about: (picked[0] != 0 && picked[1] == 0) || (picked[0] == 0 && picked[1] !=0) ?

Done.

Comments from Reviewable

tests - TestStickyKeyFromContext - TestStickinessEnd2end - service config value is read correctly - TestStickinessChangeMDKey - TestStickinessSwitchingBalancer add doc

- comment for missing field in service config - atomic swap pointer - XOR in test

This reverts commit e8a6e28.

dfawley requested a review from lyuxuan April 5, 2018 20:15

dfawley assigned MakMukhi Apr 5, 2018

dfawley requested a review from MakMukhi April 5, 2018 20:15

dfawley assigned lyuxuan Apr 5, 2018

MakMukhi suggested changes Apr 10, 2018

View reviewed changes

menghanl force-pushed the stickiness branch from ae8fb1e to d592f68 Compare April 10, 2018 22:59

menghanl commented Apr 10, 2018

View reviewed changes

menghanl assigned menghanl and unassigned lyuxuan and MakMukhi Apr 12, 2018

menghanl force-pushed the stickiness branch 3 times, most recently from e038967 to 21f4ca0 Compare April 16, 2018 23:12

MakMukhi approved these changes Apr 18, 2018

View reviewed changes

menghanl added 7 commits April 23, 2018 14:03

stickiness

a49a832

tests - TestStickyKeyFromContext - TestStickinessEnd2end - service config value is read correctly - TestStickinessChangeMDKey - TestStickinessSwitchingBalancer add doc

fix review comments

30d1a22

use atomic.Value

176f7da

rename service config field to stickinessMetadataKey

2bdf3e1

guard stickiness by env var GRPC_GO_STICKINESS

3d44087

add a TODO for limiting number of entries

fef2978

fix service config change race

689e29b

menghanl force-pushed the stickiness branch from 910005e to 64d0ccc Compare April 23, 2018 21:03

fix review comments

1a13374

- comment for missing field in service config - atomic swap pointer - XOR in test

menghanl force-pushed the stickiness branch from 64d0ccc to 1a13374 Compare April 23, 2018 22:08

menghanl merged commit e8a6e28 into grpc:master Apr 24, 2018

menghanl deleted the stickiness branch April 24, 2018 17:37

menghanl added this to the 1.12 Release milestone Apr 24, 2018

menghanl added the Type: Feature New features or improvements in behavior label Apr 24, 2018

menghanl added a commit to menghanl/grpc-go that referenced this pull request Jun 25, 2018

Revert "stickiness: add stickiness support (grpc#1969)"

e7ad0a2

This reverts commit e8a6e28.

lock bot locked as resolved and limited conversation to collaborators Oct 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stickiness: add stickiness support #1969

stickiness: add stickiness support #1969

menghanl commented Apr 5, 2018 •

edited

Loading

MakMukhi Apr 10, 2018

menghanl Apr 10, 2018

MakMukhi Apr 10, 2018

menghanl Apr 10, 2018

MakMukhi Apr 10, 2018

menghanl Apr 10, 2018

MakMukhi Apr 10, 2018

menghanl Apr 10, 2018

MakMukhi Apr 10, 2018

menghanl Apr 10, 2018

menghanl left a comment

menghanl Apr 10, 2018

menghanl Apr 10, 2018

menghanl Apr 10, 2018

menghanl Apr 10, 2018

menghanl Apr 10, 2018

menghanl commented Apr 18, 2018 •

edited

Loading

MakMukhi commented Apr 18, 2018

MakMukhi commented Apr 18, 2018

MakMukhi commented Apr 18, 2018

menghanl commented Apr 18, 2018

stickiness: add stickiness support #1969

stickiness: add stickiness support #1969

Conversation

menghanl commented Apr 5, 2018 • edited Loading

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

menghanl left a 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

menghanl commented Apr 18, 2018 • edited Loading

MakMukhi commented Apr 18, 2018

MakMukhi commented Apr 18, 2018

MakMukhi commented Apr 18, 2018

menghanl commented Apr 18, 2018

menghanl commented Apr 5, 2018 •

edited

Loading

menghanl commented Apr 18, 2018 •

edited

Loading