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

Make heavy loaded optimization configurable #114

Closed
wants to merge 2 commits into from
+32 −4
Closed

Make heavy loaded optimization configurable #114

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75e84e0
Fix check in maybeReplaceWithLessBusyConnection
avelanarius Feb 16, 2023
369ef58
Make heavy loaded optimization configurable
avelanarius Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,6 +1537,10 @@ func (c *Conn) AvailableStreams() int {
return c.streams.Available()
}

func (c *Conn) InUseStreams() int {
return c.streams.InUse()
}

func (c *Conn) UseKeyspace(keyspace string) error {
q := &writeQueryFrame{statement: `USE "` + keyspace + `"`}
q.params.consistency = c.session.cons
Expand Down
4 changes: 4 additions & 0 deletions internal/streams/streams.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
6 changes: 3 additions & 3 deletions scylla.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,10 +345,10 @@ func (p *scyllaConnPicker) maybeReplaceWithLessBusyConnection(c *Conn) *Conn {
return c
}
alternative := p.leastBusyConn()
if alternative == nil || alternative.AvailableStreams() * 120 > c.AvailableStreams() * 100 {
return c
} else {
if alternative != nil && alternative.InUseStreams() * 100 < c.InUseStreams() * 80 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link

@vladzcloudius vladzcloudius Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Got it.

return alternative
} else {
return c
}
}

Expand Down