[#275] client: Return status from all methods #275

mbiryukova · 2024-09-24T13:37:47Z

mbiryukova commented

2024-09-24 13:37:47 +00:00

Since status is checked first in handleError method, it should be returned from client methods

Signed-off-by: Marina Biryukova m.biryukova@yadro.com

Since status is checked first in handleError method, it should be returned from client methods Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova self-assigned this 2024-09-24 13:37:47 +00:00

mbiryukova added 1 commit 2024-09-24 13:37:48 +00:00

[#xxx] client: Return status from all methods

DCO / DCO (pull_request) Failing after 40s

Details

Tests and linters / Tests (pull_request) Successful in 1m9s

Details

Tests and linters / Lint (pull_request) Successful in 1m44s

Details

3063b525d5

Since status is checked first in handleError method, it should be returned from client methods

Signed-off-by: Marina Biryukova <m.biryukova@yadro.com>

mbiryukova force-pushed bugfix/client_status from 3063b525d5 to 1725c2ca13

2024-09-24 13:38:06 +00:00

Compare

mbiryukova changed title from ~~[#xxx] client: Return status from all methods~~ to [#275] client: Return status from all methods

2024-09-24 13:38:33 +00:00

mbiryukova requested review from storage-core-committers 2024-09-24 13:40:37 +00:00

mbiryukova requested review from storage-core-developers 2024-09-24 13:40:38 +00:00

mbiryukova requested review from storage-sdk-committers 2024-09-24 13:40:39 +00:00

mbiryukova requested review from storage-sdk-developers 2024-09-24 13:40:40 +00:00

mbiryukova requested review from storage-services-committers 2024-09-24 13:40:41 +00:00

mbiryukova requested review from storage-services-developers 2024-09-24 13:40:41 +00:00

dkirillov reviewed 2024-09-24 14:24:39 +00:00

client/netmap.go Outdated

					
				@ -240,7 +240,7 @@ func (c *Client) NetMapSnapshot(ctx context.Context, _ PrmNetMapSnapshot) (*ResN

					var res ResNetMapSnapshot

					res.st, err = c.processResponse(resp)

					if err != nil {

dkirillov commented

2024-09-24 14:24:38 +00:00

Should we use if err != nil || !apistatus.IsSuccessful(res.st) ? Probably we need someone from core team

Should we use `if err != nil || !apistatus.IsSuccessful(res.st)` ? Probably we need someone from `core` team

dkirillov commented

2024-09-24 14:26:02 +00:00

Oh, I see next line (246). So suppose we can freely use if err != nil || !apistatus.IsSuccessful(res.st) and drop lines 246-248

Oh, I see next line (246). So suppose we can freely use `if err != nil || !apistatus.IsSuccessful(res.st)` and drop lines 246-248

👍 1

dkirillov marked this conversation as resolved

mbiryukova force-pushed bugfix/client_status from 1725c2ca13 to 1b67ab9608

2024-09-24 15:29:55 +00:00

Compare

dkirillov approved these changes 2024-09-25 06:36:43 +00:00

acid-ant approved these changes 2024-09-25 12:20:26 +00:00

alexvanin approved these changes 2024-09-25 12:30:18 +00:00

elebedeva approved these changes 2024-09-25 12:46:39 +00:00

aarifullin reviewed 2024-09-25 12:58:42 +00:00

client/object_put_raw.go

					
				@ -162,3 +159,1 @@

					if !apistatus.IsSuccessful(x.res.st) {

						return &x.res, nil

					if x.err != nil || !apistatus.IsSuccessful(x.res.st) {

aarifullin commented

2024-09-25 12:58:42 +00:00

It seems that then the errors may not be distinguished: either the error occurred by performing a request (signature check failure) or by processing of request (APE check failure while PutObject handling).
Although, we have such an error resolution:

func (c *Client) processResponse(resp responseV2) (apistatus.Status, error) {
	if c.prm.ResponseInfoCallback != nil {
		rmi := ResponseMetaInfo{
			key:   resp.GetVerificationHeader().GetBodySignature().GetKey(),
			epoch: resp.GetMetaHeader().GetEpoch(),
		}
		if err := c.prm.ResponseInfoCallback(rmi); err != nil {
			return nil, fmt.Errorf("response callback error: %w", err)
		}
	}

	err := signature.VerifyServiceMessage(resp)
	if err != nil {
		return nil, fmt.Errorf("invalid response signature: %w", err)
	}

	st := apistatus.FromStatusV2(resp.GetMetaHeader().GetStatus())
	if !c.prm.DisableFrostFSErrorResolution {
		return st, apistatus.ErrFromStatus(st)
	}
	return st, nil
}

that doesn't seem controversial to the introduced approach

It seems that then the errors may **not** be distinguished: either the error occurred by _performing_ a request (signature check failure) or by _processing_ of request (APE check failure while `PutObject` handling). Although, we have such an error resolution: ```go func (c *Client) processResponse(resp responseV2) (apistatus.Status, error) { if c.prm.ResponseInfoCallback != nil { rmi := ResponseMetaInfo{ key: resp.GetVerificationHeader().GetBodySignature().GetKey(), epoch: resp.GetMetaHeader().GetEpoch(), } if err := c.prm.ResponseInfoCallback(rmi); err != nil { return nil, fmt.Errorf("response callback error: %w", err) } } err := signature.VerifyServiceMessage(resp) if err != nil { return nil, fmt.Errorf("invalid response signature: %w", err) } st := apistatus.FromStatusV2(resp.GetMetaHeader().GetStatus()) if !c.prm.DisableFrostFSErrorResolution { return st, apistatus.ErrFromStatus(st) } return st, nil } ``` that doesn't seem controversial to the introduced approach