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

Add node addresses as debug information #147

Merged
fyrchik merged 1 commit from ironbee/frostfs-sdk-go:extend_debug_info into master 2024-09-04 19:51:15 +00:00
Conversation 0 Commits 1 Files changed 1 +66 -14
ironbee commented 2023-08-11 11:00:51 +00:00
Contributor
Copy link

Signed-off-by: Artem Tataurov a.tataurov@yadro.com

Closes #51

Signed-off-by: Artem Tataurov <a.tataurov@yadro.com> Closes #51
ironbee requested review from dkirillov 2023-08-11 11:01:16 +00:00
ironbee requested review from storage-services-committers 2023-08-11 11:01:16 +00:00
ironbee requested review from storage-services-developers 2023-08-11 11:01:16 +00:00
ironbee requested review from storage-core-committers 2023-08-11 11:01:17 +00:00
ironbee requested review from storage-core-developers 2023-08-11 11:01:17 +00:00
ironbee force-pushed extend_debug_info from 0cfcbc32ea to 03100bd6f7 2023-08-11 11:12:12 +00:00 Compare
dstepanov-yadro requested changes 2023-08-11 12:29:11 +00:00
pool/pool.go Outdated
@ -692,3 +700,3 @@
}
return oid.ID{}, fmt.Errorf("read payload: %w", c.handleError(ctx, nil, err))
err = c.handleError(ctx, nil, err)
dstepanov-yadro commented 2023-08-11 12:26:08 +00:00
Member
Copy link

Here must be nil check: if err = c.handleError(ctx, nil, err); err != nil

Here must be nil check: ```if err = c.handleError(ctx, nil, err); err != nil```
ironbee commented 2023-08-11 13:51:28 +00:00
Author
Contributor
Copy link

There was no check initially and the c.handleError(ctx, nil, err) call used "as is". Although, it handles the nil error. I added a nil check to all wrapError methods instead.

There was no check initially and the `c.handleError(ctx, nil, err)` call used "as is". Although, it handles the nil error. I added a nil check to all wrapError methods instead.
dstepanov-yadro marked this conversation as resolved
pool/pool.go Outdated
@ -2485,2 +2495,4 @@
nodes := make([]string, 0, len(inner.clients))
inner.lock.RLock()
for _, cl := range inner.clients {
if cl.isHealthy() {
dstepanov-yadro commented 2023-08-11 12:29:06 +00:00
Member
Copy link

@fyrchik usually says: 'unrelated to commit'. It must be separate commit, as this change doesn't relate to Add node addresses as debug information

@fyrchik usually says: 'unrelated to commit'. It must be separate commit, as this change doesn't relate to `Add node addresses as debug information`
🚀 1
dstepanov-yadro marked this conversation as resolved
ironbee force-pushed extend_debug_info from 03100bd6f7 to 8ca1cd0b6c 2023-08-11 13:47:36 +00:00 Compare
ironbee force-pushed extend_debug_info from 8ca1cd0b6c to db1d3a4926 2023-08-11 13:58:01 +00:00 Compare
dkirillov reviewed 2023-08-11 14:15:05 +00:00
pool/pool.go Outdated
@ -404,3 +415,3 @@
cl, err := c.getClient()
if err != nil {
return cid.ID{}, err
return cid.ID{}, c.wrapError(err)
dkirillov commented 2023-08-11 14:15:05 +00:00
Member
Copy link

It seems we can log error only on pool level.

For example:
here we can write

func (p *Pool) PutContainer(ctx context.Context, prm PrmContainerPut) (cid.ID, error) {
	cp, err := p.connection()
	if err != nil {
		return cid.ID{}, err
	}

	cnrID, err := cp.containerPut(ctx, prm)
	if err != nil {
		return cid.ID{}, fmt.Errorf("put container via client '%s': %w", cp.address(), err)
	}
	
	return cnrID, nil
}

This can reduce number of places where we have to handle errors

It seems we can log error only on `pool` level. For example: [here](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/03827857639916df3f998a91d647eb284b7d8026/pool/pool.go#L2398) we can write ```golang func (p *Pool) PutContainer(ctx context.Context, prm PrmContainerPut) (cid.ID, error) { cp, err := p.connection() if err != nil { return cid.ID{}, err } cnrID, err := cp.containerPut(ctx, prm) if err != nil { return cid.ID{}, fmt.Errorf("put container via client '%s': %w", cp.address(), err) } return cnrID, nil } ``` This can reduce number of places where we have to handle errors
👍 1
dkirillov marked this conversation as resolved
dstepanov-yadro approved these changes 2023-08-11 15:01:09 +00:00
ironbee force-pushed extend_debug_info from db1d3a4926 to 4658a1b7e7 2023-08-14 09:13:43 +00:00 Compare
aarifullin approved these changes 2023-08-14 09:38:50 +00:00
ironbee force-pushed extend_debug_info from 4c03286359 to a9264c2d39 2023-08-14 10:08:26 +00:00 Compare
dkirillov reviewed 2023-08-14 11:26:09 +00:00
pool/pool.go Outdated
@ -2376,3 +2385,2 @@
return res, p.call(ctx, &cc, func() error {
res, err = cc.client.objectSearch(ctx, prm)
return err
if err != nil {
dkirillov commented 2023-08-14 11:26:09 +00:00
Member
Copy link

It seems the the cc.client.objectSearch call is missed

It seems the the `cc.client.objectSearch` call is missed
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-14 11:29:52 +00:00
pool/pool.go Outdated
@ -2377,2 +2386,2 @@
res, err = cc.client.objectSearch(ctx, prm)
return err
if err != nil {
return fmt.Errorf("search object via client %s: %w", cc.client.address(), err)
dkirillov commented 2023-08-14 11:29:52 +00:00
Member
Copy link

Actually here we can use cc.endpoint instead of cc.client.address() but it's up to you

Actually here we can use `cc.endpoint` instead of `cc.client.address()` but it's up to you
dkirillov marked this conversation as resolved
ironbee force-pushed extend_debug_info from a9264c2d39 to 6bea1095c6 2023-08-14 11:56:34 +00:00 Compare
dkirillov reviewed 2023-08-14 12:35:39 +00:00
pool/pool.go Outdated
@ -2151,3 +2151,3 @@
// removes session token from cache in case of token error
p.checkSessionTokenErr(err, ctxCall.endpoint)
return id, fmt.Errorf("init writing on API client: %w", err)
return id, fmt.Errorf("init writing on API client %s: %w", ctxCall.client.address(), err)
dkirillov commented 2023-08-14 12:35:39 +00:00
Member
Copy link

Why don't use ctxCall.endpoint?

Why don't use `ctxCall.endpoint`?
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-14 12:36:08 +00:00
dkirillov approved these changes 2023-08-14 12:36:31 +00:00
fyrchik approved these changes 2023-08-14 12:40:18 +00:00
acid-ant approved these changes 2023-08-14 12:43:38 +00:00
ironbee force-pushed extend_debug_info from 6bea1095c6 to 75cdfc5d9b 2023-08-14 13:23:34 +00:00 Compare
dkirillov requested changes 2023-08-14 13:34:07 +00:00
pool/statistic.go Outdated
@ -9,6 +9,7 @@ import (
type Statistic struct {
overallErrors uint64
nodes []NodeStatistic
currentNodes []string
dkirillov commented 2023-08-14 13:34:02 +00:00
Member
Copy link

We need getter for this field

We need getter for this field
dkirillov marked this conversation as resolved
ironbee changed title from Add node addresses as debug information to WIP: Add node addresses as debug information 2023-08-14 13:34:24 +00:00
ironbee force-pushed extend_debug_info from 75cdfc5d9b to 27020ff242 2023-08-15 06:38:45 +00:00 Compare
dkirillov reviewed 2023-08-15 06:43:26 +00:00
pool/statistic.go Outdated
@ -21,6 +22,11 @@ func (s Statistic) Nodes() []NodeStatistic {
return s.nodes
}
// CurrentNodes returns list of nodes of inner pool that has at least one healthy node.
dkirillov commented 2023-08-15 06:43:26 +00:00
Member
Copy link

Let's mention that these nodes have the same priority and this priority is the highest among inner pool with healthy nodes

Let's mention that these nodes have the same priority and this priority is the highest among inner pool with healthy nodes
dkirillov marked this conversation as resolved
ironbee force-pushed extend_debug_info from 27020ff242 to a3b5d4d4f5 2023-08-15 06:52:01 +00:00 Compare
ironbee changed title from WIP: Add node addresses as debug information to Add node addresses as debug information 2023-08-15 07:00:55 +00:00
dkirillov approved these changes 2023-08-15 07:17:33 +00:00
ironbee requested review from fyrchik 2023-08-15 07:27:01 +00:00
ironbee requested review from dstepanov-yadro 2023-08-15 07:57:21 +00:00
ironbee was assigned by dkirillov 2023-08-16 07:04:52 +00:00
fyrchik merged commit a3b5d4d4f5 into master 2023-08-16 07:31:37 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
aarifullin
acid-ant
dkirillov
fyrchik
dstepanov-yadro
TrueCloudLab/storage-services-developers
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
6 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#147
No description provided.
Delete branch "ironbee/frostfs-sdk-go:extend_debug_info"

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?