TrueCloudLab/frostfs-sdk-go
32
0
Fork 21
Code Issues 31 Pull requests 3 Releases Activity Actions

Refactor 'Pool' #316

Merged
fyrchik merged 7 commits from achuprov/frostfs-sdk-go:feat/refactorPool into master 2025-03-07 11:45:32 +00:00
Conversation 7 Commits 7 Files changed 6 +1766 -1684
achuprov commented 2024-12-18 16:19:01 +00:00
Member
Copy link

Close #300

This PR splits the functionality of Pool into the following components:

Pool: Provides the external interface for interaction.
connectionManager: Implements the internal functionality for managing clients.
healthCheck: Handles client health monitoring.

Close #300 This PR splits the functionality of `Pool` into the following components: `Pool`: Provides the external interface for interaction. `connectionManager`: Implements the internal functionality for managing clients. `healthCheck`: Handles client health monitoring.
achuprov added 4 commits 2024-12-18 16:19:02 +00:00 
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
[#300] pool: Move 'connectionManager' to separate file
All checks were successful
DCO / DCO (pull_request) Successful in 1m18s
Details
Tests and linters / Tests (pull_request) Successful in 3m1s
Details
Tests and linters / Lint (pull_request) Successful in 4m54s
Details
8cce85337b 
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
requested reviews from storage-core-committers, storage-core-developers, storage-services-committers, storage-services-developers 2024-12-18 16:19:02 +00:00
nzinkevich reviewed 2024-12-23 07:08:34 +00:00
pool/connection_manager.go Outdated
@ -0,0 +12,4 @@
"sync/atomic"
"time"
sdkClient "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client"
nzinkevich commented 2024-12-23 06:36:38 +00:00
Member
Copy link

nitpick: consider remove uppercase from sdkClient in all files

nitpick: consider remove uppercase from `sdkClient` in all files
fyrchik commented 2025-01-09 08:44:25 +00:00
Owner
Copy link

It is a copypaste, I would leave as-is.

It is a copypaste, I would leave as-is.
👍 1
pool/pool.go Outdated
@ -1967,3 +1948,3 @@
)
// NewPool creates connection pool using parameters.
// NewPool creates cnnectionManager using parameters.
nzinkevich commented 2024-12-23 07:03:33 +00:00
Member
Copy link

Typo. Overall I would change comment a bit, e.g. NewPool inits a connectionManager and returns a new Pool instance. Same for newConnectionManager.

Typo. Overall I would change comment a bit, e.g. `NewPool inits a connectionManager and returns a new Pool instance`. Same for `newConnectionManager`.
fyrchik commented 2024-12-23 08:23:12 +00:00
Owner
Copy link

It NewPool creates connectionManager, what does newConnectionManager do?

It `NewPool` creates `connectionManager`, what does `newConnectionManager` do?
achuprov commented 2025-03-05 15:49:44 +00:00
Author
Member
Copy link

Typo. Overall I would change comment a bit, e.g. NewPool inits a connectionManager and returns a new Pool instance. Same for newConnectionManager.

Fixed. I think the comment for NewPool should not contain information about private components of Pool.

>Typo. Overall I would change comment a bit, e.g. NewPool inits a connectionManager and returns a new Pool instance. Same for newConnectionManager. Fixed. I think the comment for `NewPool` should not contain information about private components of Pool.
achuprov commented 2025-03-05 15:55:08 +00:00
Author
Member
Copy link

It NewPool creates connectionManager, what does newConnectionManager do?

Line 32 in 55fd24e
// newConnectionManager returns an instance of connectionManager configured according to the parameters.

> It NewPool creates connectionManager, what does newConnectionManager do? https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/55fd24e156925eb9150e95bd84d4122b6f69124d/pool/connection_manager.go#L32
nzinkevich reviewed 2024-12-23 07:26:38 +00:00
pool/pool.go Outdated
@ -2688,19 +2190,19 @@ func (p *Pool) GetObject(ctx context.Context, prm PrmObjectGet) (ResGetObject, e
//
// Main return value MUST NOT be processed on an erroneous return.
nzinkevich commented 2024-12-23 07:26:38 +00:00
Member
Copy link

question: Are these comments (here and in other methods) necessary? I believe it's clear that we should not process the result if an error occurs

question: Are these comments (here and in other methods) necessary? I believe it's clear that we should not process the result if an error occurs
achuprov commented 2025-03-05 15:51:20 +00:00
Author
Member
Copy link

Fixed

Fixed
achuprov was assigned by dkirillov 2024-12-23 13:25:08 +00:00
dkirillov commented 2024-12-25 09:45:52 +00:00
Member
Copy link

In general, LGTM but I have feeling that these entities (Pool, connectionManager and healthchek) are still tight coupled, know a lot about each other (and share responsibility).

As I understand the main idea is getting from connection manager ready client to use. I would expect we form some parameters (prmContext is just fine) and invoke method (initCallContext is also just fine) to get client.
But

I suggest we should make all these struct more loosely coupled. To make it easier to understand responsibility we can place (maybe just during development) each struct into separate package and work with it know nothing about inner struct and just using exported methods

In general, LGTM but I have feeling that these entities (`Pool`, `connectionManager` and `healthchek`) are still tight coupled, know a lot about each other (and share responsibility). * `healthcheck` struct knows about buffers, node params https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/healthcheck.go#L23 though it just runs `callback` by ticker https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/healthcheck.go#L41-L43 * `Pool` struct knows a lot about `connectionManager`. Let's look at `PutObject` https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2054 As I understand the main idea is getting from connection manager ready client to use. I would expect we form some parameters ([prmContext](https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2057) is just fine) and invoke method ([initCallContext](https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2066) is also just fine) to get client. But * we get key from manager (we know that it has key) https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2062 * explicitly open default session (we know that it has such method) https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2072 * we explicitly use cache (unexported field) from `manager` https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2079 * we use very specific error handler from `manager` https://git.frostfs.info/achuprov/frostfs-sdk-go/src/commit/8cce85337b157c0fbafa08ef5e0650f984da0817/pool/pool.go#L2087 I suggest we should make all these struct more loosely coupled. To make it easier to understand responsibility we can place (maybe just during development) each struct into separate package and work with it know nothing about inner struct and just using exported methods
achuprov force-pushed feat/refactorPool from 8cce85337b to 2df73145df 2025-02-11 16:04:58 +00:00 Compare
achuprov force-pushed feat/refactorPool from 2df73145df to b306cf06e4 2025-02-12 15:13:37 +00:00 Compare
achuprov commented 2025-02-12 16:03:03 +00:00
Author
Member
Copy link

@dkirillov

healthcheck struct knows about buffers, node params

Now, only get the callback with ctx.

we get key from manager (we know that it has key)

Move call fillAppropriateKey(&prm.prmCommon to initCall

we explicitly use cache (unexported field) from manager

Move manager.cache.Epoch() to manager.Epoch()

we use very specific error handler from manager

fixed

diff

@dkirillov > healthcheck struct knows about buffers, node params Now, only get the callback with ctx. > we get key from manager (we know that it has key) Move call `fillAppropriateKey(&prm.prmCommon` to `initCall` > we explicitly use cache (unexported field) from manager Move `manager.cache.Epoch()` to `manager.Epoch()` > we use very specific error handler from manager fixed [diff](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/compare/8cce85337b157c0fbafa08ef5e0650f984da0817..b306cf06e41534253806a6fdfda6552d3fd80aaf)
dkirillov commented 2025-02-20 13:26:55 +00:00
Member
Copy link

Maybe we can keep all initCall logic in Pool. And connectionManager will just return healthy client/connection?

Maybe we can keep all `initCall` logic in `Pool`. And `connectionManager` will just return healthy client/connection?
achuprov force-pushed feat/refactorPool from b306cf06e4 to 7a2b153c11 2025-02-25 15:23:17 +00:00 Compare
achuprov commented 2025-02-25 15:30:26 +00:00
Author
Member
Copy link

@dkirillov Ok, all session management logic is left in Pool.
connectionManager returns only client and statistic

@dkirillov Ok, all session management logic is left in `Pool`. `connectionManager` returns only `client` and `statistic`
dkirillov reviewed 2025-02-27 08:00:05 +00:00
dkirillov left a comment
Member
Copy link

Since we do refactor, can we also move clientWrapper (with all its methods and related structs (clientStatusMonitor, wrapperPrm)) to separate file (client.go for example)?

Since we do refactor, can we also move `clientWrapper` (with all its methods and related structs (`clientStatusMonitor`, `wrapperPrm`)) to separate file (`client.go` for example)?
pool/connection_manager.go Outdated
@ -0,0 +261,4 @@
// Iterate iterates over all clients in the connectionManager. It returns false if there are no more clients.
// User MUST fully consume the iterator until false is received.
func (cm *connectionManager) iterate() func() (client, bool) {
dkirillov commented 2025-02-27 07:55:55 +00:00
Member
Copy link

Why don't we use something like:

diff --git a/pool/connection_manager.go b/pool/connection_manager.go
index 505edc7..833a1a5 100644
--- a/pool/connection_manager.go
+++ b/pool/connection_manager.go
@@ -261,27 +261,12 @@ func (cm *connectionManager) connection() (client, error) {
 
 // Iterate iterates over all clients in the connectionManager. It returns false if there are no more clients.
 // User MUST fully consume the iterator until false is received.
-func (cm *connectionManager) iterate() func() (client, bool) {
-	i := 0
-	var f func() (client, bool)
-	return func() (client, bool) {
-		if f == nil {
-			if i == len(cm.innerPools) {
-				return nil, false
-			}
-			f = cm.innerPools[i].iterate()
-		}
-
-		for {
-			res, ok := f()
-			if ok {
-				return res, true
-			}
-			i++
-			if i == len(cm.innerPools) {
-				return nil, false
+func (cm *connectionManager) iterate(cb func(client)) {
+	for _, inner := range cm.innerPools {
+		for _, cl := range inner.clients {
+			if cl.isHealthy() {
+				cb(cl)
 			}
-			f = cm.innerPools[i].iterate()
 		}
 	}
 }
@@ -307,29 +292,6 @@ func (p *innerPool) connection() (client, error) {
 	return nil, errors.New("no healthy client")
 }
 
-// Iterate iterates over all clients in the innerPool. It returns false if there are no more clients.
-// User MUST fully consume the iterator until false is received.
-func (p *innerPool) iterate() func() (client, bool) {
-	i := 0
-	p.lock.RLock()
-	done := false
-	return func() (client, bool) {
-		if done {
-			return nil, false
-		}
-
-		if i < len(p.clients) {
-			cl := p.clients[i]
-			i++
-			return cl, true
-		}
-
-		done = false
-		p.lock.RUnlock()
-		return nil, false
-	}
-}
-
 func (cm connectionManager) Statistic() Statistic {
 	stat := Statistic{}
 	for _, inner := range cm.innerPools {
diff --git a/pool/pool.go b/pool/pool.go
index 91b2b36..d5fb01d 100644
--- a/pool/pool.go
+++ b/pool/pool.go
@@ -1993,12 +1993,7 @@ func (p *Pool) Dial(ctx context.Context) error {
 		return err
 	}
 
-	it := p.manager.iterate()
-	for {
-		cl, ok := it()
-		if !ok {
-			break
-		}
+	p.manager.iterate(func(cl client) {
 		var st session.Object
 		err := initSessionForDuration(ctx, &st, cl, p.manager.rebalanceParams.sessionExpirationDuration, *p.key, false)
 		if err != nil {
@@ -2007,11 +2002,11 @@ func (p *Pool) Dial(ctx context.Context) error {
 				p.logger.Log(zap.WarnLevel, "failed to create frostfs session token for client",
 					zap.String("address", cl.address()), zap.Error(err))
 			}
-			continue
+			return
 		}
 
 		_ = p.cache.Put(formCacheKey(cl.address(), p.key, false), st)
-	}
+	})
 
 	ni, err := p.NetworkInfo(ctx)
 	if err != nil {
Why don't we use something like: ```diff diff --git a/pool/connection_manager.go b/pool/connection_manager.go index 505edc7..833a1a5 100644 --- a/pool/connection_manager.go +++ b/pool/connection_manager.go @@ -261,27 +261,12 @@ func (cm *connectionManager) connection() (client, error) { // Iterate iterates over all clients in the connectionManager. It returns false if there are no more clients. // User MUST fully consume the iterator until false is received. -func (cm *connectionManager) iterate() func() (client, bool) { - i := 0 - var f func() (client, bool) - return func() (client, bool) { - if f == nil { - if i == len(cm.innerPools) { - return nil, false - } - f = cm.innerPools[i].iterate() - } - - for { - res, ok := f() - if ok { - return res, true - } - i++ - if i == len(cm.innerPools) { - return nil, false +func (cm *connectionManager) iterate(cb func(client)) { + for _, inner := range cm.innerPools { + for _, cl := range inner.clients { + if cl.isHealthy() { + cb(cl) } - f = cm.innerPools[i].iterate() } } } @@ -307,29 +292,6 @@ func (p *innerPool) connection() (client, error) { return nil, errors.New("no healthy client") } -// Iterate iterates over all clients in the innerPool. It returns false if there are no more clients. -// User MUST fully consume the iterator until false is received. -func (p *innerPool) iterate() func() (client, bool) { - i := 0 - p.lock.RLock() - done := false - return func() (client, bool) { - if done { - return nil, false - } - - if i < len(p.clients) { - cl := p.clients[i] - i++ - return cl, true - } - - done = false - p.lock.RUnlock() - return nil, false - } -} - func (cm connectionManager) Statistic() Statistic { stat := Statistic{} for _, inner := range cm.innerPools { diff --git a/pool/pool.go b/pool/pool.go index 91b2b36..d5fb01d 100644 --- a/pool/pool.go +++ b/pool/pool.go @@ -1993,12 +1993,7 @@ func (p *Pool) Dial(ctx context.Context) error { return err } - it := p.manager.iterate() - for { - cl, ok := it() - if !ok { - break - } + p.manager.iterate(func(cl client) { var st session.Object err := initSessionForDuration(ctx, &st, cl, p.manager.rebalanceParams.sessionExpirationDuration, *p.key, false) if err != nil { @@ -2007,11 +2002,11 @@ func (p *Pool) Dial(ctx context.Context) error { p.logger.Log(zap.WarnLevel, "failed to create frostfs session token for client", zap.String("address", cl.address()), zap.Error(err)) } - continue + return } _ = p.cache.Put(formCacheKey(cl.address(), p.key, false), st) - } + }) ni, err := p.NetworkInfo(ctx) if err != nil { ```
achuprov commented 2025-03-04 16:57:57 +00:00
Author
Member
Copy link

You are right, your solution is more elegant.
Final diff

You are right, your solution is more elegant. Final [diff](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/compare/7a2b153c11b0c127b65f302ffa4a6a708bac57ad..70a543746b72dc126828c4ee1bee4ca9df017463)
dkirillov marked this conversation as resolved
pool/connection_manager.go Outdated
@ -0,0 +334,4 @@
stat := Statistic{}
for _, inner := range cm.innerPools {
nodes := make([]string, 0, len(inner.clients))
inner.lock.RLock()
dkirillov commented 2025-02-27 07:49:28 +00:00
Member
Copy link

It seems we don't need to use mutex here. Each client thread-safe and mutext protect only inner.sampler

It seems we don't need to use mutex here. Each client thread-safe and mutext protect only `inner.sampler`
achuprov commented 2025-03-04 16:54:24 +00:00
Author
Member
Copy link

Done

Done
dkirillov marked this conversation as resolved
achuprov force-pushed feat/refactorPool from 7a2b153c11 to ad36d13b3b 2025-03-04 15:19:25 +00:00 Compare
achuprov force-pushed feat/refactorPool from ad36d13b3b to 43c6924116 2025-03-04 15:22:36 +00:00 Compare
achuprov force-pushed feat/refactorPool from 43c6924116 to 656727e79b 2025-03-04 15:33:43 +00:00 Compare
achuprov force-pushed feat/refactorPool from 656727e79b to 70a543746b 2025-03-04 16:51:44 +00:00 Compare
dkirillov approved these changes 2025-03-05 06:39:59 +00:00
Dismissed
dkirillov left a comment
Member
Copy link

LGTM,
but what do you think about #316 (comment)

LGTM, but what do you think about https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/316#issuecomment-68615
achuprov force-pushed feat/refactorPool from 70a543746b to 143be39782 2025-03-05 13:11:56 +00:00 Compare
achuprov dismissed dkirillov's review 2025-03-05 13:11:57 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

achuprov force-pushed feat/refactorPool from 143be39782 to be26e50fb5 2025-03-05 15:01:30 +00:00 Compare
achuprov force-pushed feat/refactorPool from be26e50fb5 to 48262e937f 2025-03-05 15:02:01 +00:00 Compare
achuprov force-pushed feat/refactorPool from 48262e937f to 6a3bf114ea 2025-03-05 15:02:22 +00:00 Compare
achuprov force-pushed feat/refactorPool from 6a3bf114ea to fc8aebdb25 2025-03-05 15:18:57 +00:00 Compare
achuprov commented 2025-03-05 15:21:00 +00:00
Author
Member
Copy link

⬆️ Add containerListStream

⬆️ Add `containerListStream`
achuprov force-pushed feat/refactorPool from fc8aebdb25 to ac8b8b4b47 2025-03-05 15:25:15 +00:00 Compare
achuprov commented 2025-03-05 15:25:49 +00:00
Author
Member
Copy link

⬆️ Rename field Account to OwnerID in PrmContainerList

⬆️ Rename field Account to OwnerID in PrmContainerList
achuprov force-pushed feat/refactorPool from ac8b8b4b47 to e554b6b255 2025-03-05 15:29:40 +00:00 Compare
achuprov force-pushed feat/refactorPool from e554b6b255 to 55fd24e156 2025-03-05 15:31:33 +00:00 Compare
fyrchik approved these changes 2025-03-06 06:49:12 +00:00
fyrchik commented 2025-03-06 06:50:07 +00:00
Owner
Copy link

What is with this commit message 55fd24e156 ?

What is with this commit message https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/commit/55fd24e156925eb9150e95bd84d4122b6f69124d ?
achuprov force-pushed feat/refactorPool from 55fd24e156 to 7017ae6b28 2025-03-06 14:08:26 +00:00 Compare
dkirillov approved these changes 2025-03-07 11:15:47 +00:00
nzinkevich approved these changes 2025-03-07 11:20:51 +00:00
fyrchik merged commit f70c0c9081 into master 2025-03-07 11:45:32 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
dkirillov
nzinkevich
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
pool

Tasks related to client pool

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
pool
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
achuprov
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-sdk-go#316
No description provided.
Delete branch "achuprov/frostfs-sdk-go:feat/refactorPool"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?