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.

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

pool: add external load balancing methods support #401

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KaymeKaydex
Copy link

@KaymeKaydex KaymeKaydex commented Jul 18, 2024

I added the ability to use custom balancing methods when connecting with a pool.

Related issues: #400

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since all API is moved to the public scope you need to change a package to test from a test one package pool_test to a package one package pool. And test only the public API.

Copy link
Author

Choose a reason for hiding this comment

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

done

Comment on lines +20 to +35
func NewRoundRobinStrategy(size int) BalancingPool {
return &RoundRobinStrategy{
conns: make([]*tarantool.Connection, 0, size),
indexById: make(map[string]uint, size),
size: 0,
current: 0,
}
}

func (r *roundRobinStrategy) GetConnection(id string) *tarantool.Connection {
func (r *RoundRobinStrategy) GetConnection(id string) *tarantool.Connection {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, add comments to the public API methods.

Comment on lines 7 to 14
type BalancingPool interface {
IsEmpty() bool
GetConnection(string) *tarantool.Connection
DeleteConnection(string) *tarantool.Connection
AddConnection(id string, conn *tarantool.Connection)
GetNextConnection() *tarantool.Connection
GetConnections() map[string]*tarantool.Connection
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a public API now, we need to be careful with the names. Please, add a comments for the public API too.

Suggested change
type BalancingPool interface {
IsEmpty() bool
GetConnection(string) *tarantool.Connection
DeleteConnection(string) *tarantool.Connection
AddConnection(id string, conn *tarantool.Connection)
GetNextConnection() *tarantool.Connection
GetConnections() map[string]*tarantool.Connection
}
type Balancer interface {
IsEmpty() bool
Add(id string, conn *tarantool.Connection)
Get(string) *tarantool.Connection
GetNext() *tarantool.Connection
GetAll() map[string]*tarantool.Connection
Delete(string) *tarantool.Connection
}

type BalancingMethod = func(int) BalancingPool

type BalancingPool interface {
IsEmpty() bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to remove the method from the interface since it could be replaced with:

GetNextConnection() == nil or len(GetConnections())== 0.

Suggested change
IsEmpty() bool

// ConnectedNow gets connected status of pool.
func (p *ConnectionPool) ConnectedNow(mode Mode) (bool, error) {
p.poolsMutex.RLock()
defer p.poolsMutex.RUnlock()
if p.state.get() != connectedState {
return false, nil
}
switch mode {
case ANY:
return !p.anyPool.IsEmpty(), nil
case RW:
return !p.rwPool.IsEmpty(), nil
case RO:
return !p.roPool.IsEmpty(), nil
case PreferRW:
fallthrough
case PreferRO:
return !p.rwPool.IsEmpty() || !p.roPool.IsEmpty(), nil
default:
return false, ErrNoHealthyInstance
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, rename the file to balancer.go.

Copy link
Author

Choose a reason for hiding this comment

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

done


import "github.com/tarantool/go-tarantool/v2"

type BalancingMethod = func(int) BalancingPool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are talking about a general interface, this micro-optimization with size is only confusing.

Suggested change
type BalancingMethod = func(int) BalancingPool
type BalancingMethod = func() BalancingPool


import "github.com/tarantool/go-tarantool/v2"

type BalancingMethod = func(int) BalancingPool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer an interface instead of a method. An interface could be used as a method, but doing the opposite is more difficult. But this is debatable.

type BalancerFactory interface {
    Create() Balancer
}

As additional we could pass some useful info into the method:

type BalancerFactory interface {
    Create(role Role) Balancer
}

Or we can keep it simple.

@@ -81,6 +81,8 @@ type Opts struct {
CheckTimeout time.Duration
// ConnectionHandler provides an ability to handle connection updates.
ConnectionHandler ConnectionHandler
// BalancingMethod is how connections for the request will be selected

Choose a reason for hiding this comment

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

Suggested change
// BalancingMethod is how connections for the request will be selected
// BalancingMethod determines how connections for the request will be selected.

Please make sure that all new comments end with a dot (this is part of a Tarantool code style).

@KaymeKaydex
Copy link
Author

Thanks for the review, unfortunately, I was able to return to work only at the end of August

@oleg-jukovec
Copy link
Collaborator

Thanks for the review, unfortunately, I was able to return to work only at the end of August

Thank you, it's ok. We are not planning any work on the connector.

 I added the ability to use custom balancing methods when connecting with a pool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants