[#73] pool/tree: Support pool for tree service #90

Merged
alexvanin merged 1 commits from dkirillov/frostfs-sdk-go:feature/73-add_pool_for_tree_endpoints into master 2023-07-26 21:08:02 +00:00
Collaborator

close #73

close #73
dkirillov self-assigned this 2023-06-05 15:02:25 +00:00
dkirillov force-pushed feature/73-add_pool_for_tree_endpoints from e7db3c80e1 to f9237c2755 2023-06-06 06:31:21 +00:00 Compare
dkirillov changed title from WIP: [#73] pool/tree: Support pool for tree service to [#73] pool/tree: Support pool for tree service 2023-06-06 06:31:31 +00:00
dkirillov requested review from storage-core-committers 2023-06-06 06:31:39 +00:00
dkirillov requested review from storage-core-developers 2023-06-06 06:31:39 +00:00
dkirillov requested review from storage-services-committers 2023-06-06 06:31:39 +00:00
dkirillov requested review from storage-services-developers 2023-06-06 06:31:39 +00:00
dstepanov-yadro reviewed 2023-06-07 06:46:21 +00:00
@ -0,0 +76,4 @@
c.mu.RLock()
healthy := c.healthy
dialed := c.conn != nil
c.mu.RUnlock()

It would look a lot easier with defer.

It would look a lot easier with `defer`.
Poster
Collaborator

I tried to minimize the critical section as much as possible. But probably in this particular case overhead isn't significant (if any). I'll fix it

I tried to minimize the critical section as much as possible. But probably in this particular case overhead isn't significant (if any). I'll fix it
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-06-07 07:00:07 +00:00
@ -0,0 +186,4 @@
for i, nodes := range p.rebalanceParams.nodesGroup {
clients := make([]client, len(nodes))
for j, node := range nodes {
clients[j] = newTreeClient(node.Address(), grpc.WithTransportCredentials(insecure.NewCredentials()))

Since this is a public SDK, we need to provide the user with the ability to transmit []gprs.DialOption

Since this is a public SDK, we need to provide the user with the ability to transmit `[]gprs.DialOption`
Poster
Collaborator

There is an separate issue for that #84

There is an separate issue for that https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/issues/84
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-06-07 07:02:23 +00:00
@ -0,0 +205,4 @@
}
ctx, cancel := context.WithCancel(ctx)
p.cancel = cancel

Where is p.cancel used?

Where is `p.cancel` used?
Poster
Collaborator

Forgot to add Pool.Close method

Forgot to add `Pool.Close` method
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-06-07 07:03:41 +00:00
@ -0,0 +206,4 @@
ctx, cancel := context.WithCancel(ctx)
p.cancel = cancel
p.closedCh = make(chan struct{})

Who is p.closeCh listener?

Who is `p.closeCh` listener?
dstepanov-yadro marked this conversation as resolved
dkirillov force-pushed feature/73-add_pool_for_tree_endpoints from f9237c2755 to 1c6f88ee02 2023-06-07 08:14:03 +00:00 Compare
dstepanov-yadro approved these changes 2023-06-07 14:18:11 +00:00
alexvanin reviewed 2023-06-07 16:01:31 +00:00
@ -0,0 +96,4 @@
func (c *treeClient) setHealthy(val bool) {
c.mu.Lock()
c.healthy = val
c.mu.Unlock()

Maybe defer here too like in all other functions in this file?

Maybe defer here too like in all other functions in this file?
alexvanin marked this conversation as resolved
@ -0,0 +75,4 @@
logger *zap.Logger
startIndicesMtx sync.RWMutex
startIndices [2]int

It would be nice to provide some comment to the next developer, what those indices are, why there are only two of them, etc. Pool component may be really confusing sometimes, so we will benefit from some implementation details in comments.

It would be nice to provide some comment to the next developer, what those indices are, why there are only two of them, etc. Pool component may be really confusing sometimes, so we will benefit from some implementation details in comments.
alexvanin marked this conversation as resolved
@ -0,0 +534,4 @@
for _, cl := range group.clients {
if closeErr := cl.close(); closeErr != nil {
p.log(zapcore.ErrorLevel, "close client connection", zap.Error(closeErr))
err = closeErr

I wonder if we can go for errors.Join in go1.20 here 🤔

I wonder if we can go for `errors.Join` in go1.20 here 🤔
Poster
Collaborator

With go1.20 we can write:

var err error
for _, group := range p.innerPools {
	for _, cl := range group.clients {
		err = errors.Join(err, cl.close())
	}
}
With go1.20 we can write: ```golang var err error for _, group := range p.innerPools { for _, cl := range group.clients { err = errors.Join(err, cl.close()) } } ```
@ -0,0 +720,4 @@
err = fn(cl)
}
if !shouldTryAgain(err) {
p.setStartIndices(indexI, indexJ)

setStartInidices will be invoked on every successful response and it is going to lock mutex every time. Can we do better here? Maybe update indices only if indexI and indexJ have changed in this loop?

`setStartInidices` will be invoked on every successful response and it is going to lock mutex every time. Can we do better here? Maybe update indices only if `indexI` and `indexJ` have changed in this loop?
alexvanin marked this conversation as resolved
@ -0,0 +6,4 @@
)
func (p *Pool) signData(buf []byte, f func(key, sign []byte)) error {
// When SDK library adopts Tree service client, this should be dropped.

Well, it is adopting now 😃

Well, it is adopting now 😃
alexvanin marked this conversation as resolved
dkirillov force-pushed feature/73-add_pool_for_tree_endpoints from 1c6f88ee02 to 261580c71e 2023-06-08 06:59:20 +00:00 Compare
dkirillov force-pushed feature/73-add_pool_for_tree_endpoints from 261580c71e to 19adb4dffa 2023-06-08 11:38:16 +00:00 Compare
alexvanin approved these changes 2023-06-08 12:18:30 +00:00
alexvanin merged commit 19adb4dffa into master 2023-06-08 12:19:00 +00:00
alexvanin deleted branch feature/73-add_pool_for_tree_endpoints 2023-06-08 12:19:02 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
No Milestone
No Assignees
3 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#90
There is no content yet.