Make heavy loaded optimization configurable #114

avelanarius · 2023-02-16T20:30:20Z

Scylla Go Driver has a capability to avoid sending requests to an overloaded shard, instead sending the request on a different connection (at the same node).

This PR makes it possible to customize the parameters used to determine when this behavior would kick in.

Additionally, this PR fixes an incorrect inequality when comparing the number of streams.

The inequality in maybeReplaceWithLessBusyConnection was supposed to check if the least busy connection has at least 20% less inflight requests compared to heavy loaded connection. However, this check was incorrect: if alternative == nil || alternative.AvailableStreams() * 120 > c.AvailableStreams() * 100 { return c } Since "alternative" is the least busy connection, by definition it has the largest number of available streams. Therefore, alternative.AvailableStreams() > c.AvailableStreams() is always true. This commit rewrites the condition by using a number of in use streams (inflight requests). The inequality now correctly checks is the least busy connection has at least 20% less in use streams compared to the heavy loaded connection.

Scylla Go Driver has a capability to avoid sending requests to an overloaded shard, instead sending the request on a different connection (at the same node). This change makes it possible to customize the parameters used to determine when this behavior would kick in.

avelanarius · 2023-02-17T10:37:52Z

This PR is also available at scylladb/gocql branch: heavy-loaded-optim and at scylladb/gocql tag: heavy-loaded-optim-v1.

piodul

Implementation-wise looks good, but I would really appreciate a test to verify that the optimization indeed does what was intended.

piodul · 2023-02-17T11:41:58Z

scylla.go

-	if alternative == nil || alternative.AvailableStreams() * 120 > c.AvailableStreams() * 100 {
-		return c
-	} else {
+	if alternative != nil && alternative.InUseStreams() * 100 < c.InUseStreams() * 80 {


Because of fact that the problem with the original equation wasn't spotted when it was reviewed, I would appreciate some unit tests in order to convince us better that it indeed is fixed now.

I think we already have some scyllaConnPicker tests in scylla.go.

I'm sorry but isn't AvailableStreams == NumStreams - InUseStreams?
And if so then any equation expressed in InUseStreams can be expressed in AvailableStreams and the other way around.

And this brings us back to the initial motivation of this patch.
You were absolutely right that the original expression was an always-true one.
However the mistake was not in the use of AvailableStreams vs InUseStreams but rather in the equation itself.

It was supposed to be as follows:

alternative.AvailableStreams() * 100 > c.AvailableStreams() * 120

Or in your current version:

alternative.AvailableStreams() * 80 > c.AvailableStreams() * 100 + 20 * c.NumStreams()

Don't you think that adjusting the original equation would make this patch much simpler?

I'm sorry but isn't AvailableStreams == NumStreams - InUseStreams?
And if so then any equation expressed in InUseStreams can be expressed in AvailableStreams and the other way around.

Yes.

The reason I changed it to use InUseStreams instead of AvailableStreams is that it was easier for me to understand it in terms of InUseStreams (aka inflight requests). It also makes it easier to document and explain the configurable parameters.

piodul · 2023-02-17T11:51:41Z

cluster.go

@@ -267,6 +285,8 @@ func NewCluster(hosts ...string) *ClusterConfig {
 		ConnectTimeout:         600 * time.Millisecond,
 		Port:                   9042,
 		NumConns:               2,
+		HeavyLoadedConnectionThreshold: 512,
+		HeavyLoadedSwitchConnectionPercentage: 20,


Please run gofmt on this file. It will be annoying for others contributors if gofmt keeps formatting code that they didn't modify.

piodul · 2023-02-17T11:52:29Z

cluster.go

+	// least busy connection has less than 80 inflight requests.
+	//
+	// Default: 20%
+	HeavyLoadedSwitchConnectionPercentage int


Please validate that this parameter is within valid range when the session is being created.

vladzcloudius

Good catch! Some comments below.

vladzcloudius · 2023-02-17T14:59:18Z

internal/streams/streams.go

@@ -145,3 +145,7 @@ func (s *IDGenerator) Clear(stream int) (inuse bool) {
 func (s *IDGenerator) Available() int {
 	return s.NumStreams - int(atomic.LoadInt32(&s.inuseStreams)) - 1
 }
+
+func (s *IDGenerator) InUse() int {
+	return int(atomic.LoadInt32(&s.inuseStreams))


A question from a C-guy: why did you have to create all these complications with casting like this one (int would be a int64 on 64-bit platform according to https://www.educative.io/answers/what-is-type-int-in-golang)?

I assume that the reason you need to do all these transformations is because NumStreams is int but for whatever unclear to me reason inuseStreams is int32. And this makes no sense to me at all because inuseStreams can get a value stored in NumStreams, and this implies that they should have the same type.

Am I missing something?

It is due to golang atomic package that does not have API for int.

vladzcloudius · 2023-02-17T15:12:18Z

scylla.go

-	if alternative == nil || alternative.AvailableStreams() * 120 > c.AvailableStreams() * 100 {
-		return c
-	} else {
+	if alternative != nil && alternative.InUseStreams() * 100 < c.InUseStreams() * 80 {


I'm sorry but isn't AvailableStreams == NumStreams - InUseStreams?
And if so then any equation expressed in InUseStreams can be expressed in AvailableStreams and the other way around.

And this brings us back to the initial motivation of this patch.
You were absolutely right that the original expression was an always-true one.
However the mistake was not in the use of AvailableStreams vs InUseStreams but rather in the equation itself.

It was supposed to be as follows:

alternative.AvailableStreams() * 100 > c.AvailableStreams() * 120

Or in your current version:

alternative.AvailableStreams() * 80 > c.AvailableStreams() * 100 + 20 * c.NumStreams()

Don't you think that adjusting the original equation would make this patch much simpler?

martin-sucha · 2023-02-22T11:41:30Z

Any updates?

roydahan · 2024-06-03T12:41:53Z

@vladzcloudius do you think this one is still required?
Is it still useful for anyone?

vladzcloudius · 2024-06-03T16:59:04Z

@vladzcloudius do you think this one is still required? Is it still useful for anyone?

Of course it's useful.
It is going to be useful to every Scylla Driver user.

dkropachev · 2024-06-13T04:07:03Z

@avelanarius, let us know if you be able to finish PR, otherwise someone else should pick it up

avelanarius added 2 commits February 16, 2023 21:18

avelanarius requested a review from piodul February 17, 2023 10:37

avelanarius requested a review from vladzcloudius February 17, 2023 10:52

piodul requested changes Feb 17, 2023

View reviewed changes

vladzcloudius suggested changes Feb 17, 2023

View reviewed changes

martin-sucha mentioned this pull request Feb 27, 2023

How to set pool size gocql/gocql#1679

Open

roydahan assigned avelanarius May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make heavy loaded optimization configurable #114

Make heavy loaded optimization configurable #114

avelanarius commented Feb 16, 2023

avelanarius commented Feb 17, 2023

piodul left a comment

piodul Feb 17, 2023

vladzcloudius Feb 17, 2023 •

edited

avelanarius Feb 17, 2023

vladzcloudius Feb 17, 2023

piodul Feb 17, 2023

piodul Feb 17, 2023

vladzcloudius left a comment •

edited

vladzcloudius Feb 17, 2023

dkropachev Jun 13, 2024

vladzcloudius Feb 17, 2023 •

edited

martin-sucha commented Feb 22, 2023

roydahan commented Jun 3, 2024

vladzcloudius commented Jun 3, 2024

dkropachev commented Jun 13, 2024

Make heavy loaded optimization configurable #114

Are you sure you want to change the base?

Make heavy loaded optimization configurable #114

Conversation

avelanarius commented Feb 16, 2023

avelanarius commented Feb 17, 2023

piodul left a comment

Choose a reason for hiding this comment

piodul Feb 17, 2023

Choose a reason for hiding this comment

vladzcloudius Feb 17, 2023 • edited

Choose a reason for hiding this comment

avelanarius Feb 17, 2023

Choose a reason for hiding this comment

vladzcloudius Feb 17, 2023

Choose a reason for hiding this comment

piodul Feb 17, 2023

Choose a reason for hiding this comment

piodul Feb 17, 2023

Choose a reason for hiding this comment

vladzcloudius left a comment • edited

Choose a reason for hiding this comment

vladzcloudius Feb 17, 2023

Choose a reason for hiding this comment

dkropachev Jun 13, 2024

Choose a reason for hiding this comment

vladzcloudius Feb 17, 2023 • edited

Choose a reason for hiding this comment

martin-sucha commented Feb 22, 2023

roydahan commented Jun 3, 2024

vladzcloudius commented Jun 3, 2024

dkropachev commented Jun 13, 2024

vladzcloudius Feb 17, 2023 •

edited

vladzcloudius left a comment •

edited

vladzcloudius Feb 17, 2023 •

edited