TrueCloudLab/frostfs-sdk-go
33
0
Fork 21
Code Issues 34 Pull requests 1 Releases Activity Actions

[#131] client: Switch ResolveFrostFSFailures to DisableFrostFSFailuresResolution option #163

Merged
fyrchik merged 3 commits from olefirenque/frostfs-sdk-go:131-refactor_frostfs_failure_resolving_option into master 2023-10-26 14:35:50 +00:00
Conversation 0 Commits 3 Files changed 18 +60 -57
olefirenque commented 2023-09-12 16:06:37 +00:00
Contributor
Copy link

Changed client.PrmInit.ResolveFrostFSFailures name and semantics to the opposite

Changed `client.PrmInit.ResolveFrostFSFailures` name and semantics to the opposite
aarifullin reviewed 2023-09-13 08:57:01 +00:00
client/client.go Outdated
@ -145,3 +145,3 @@
// See also Init.
type PrmInit struct {
resolveFrostFSErrors bool
dontResolveFrostFSErrors bool
aarifullin commented 2023-09-13 08:57:01 +00:00
Member
Copy link

Let's discuss:

I am not so picky about names but I would not consider the "dont" prefix for a boolean variable name.
What do you think about disableFrostFSErrorResolution?

Let's discuss: I am not so picky about names but I would not consider the "dont" prefix for a boolean variable name. What do you think about `disableFrostFSErrorResolution`?
👍 1
olefirenque commented 2023-09-13 09:20:07 +00:00
Author
Contributor
Copy link

fair enough

fair enough
olefirenque marked this conversation as resolved
aarifullin reviewed 2023-09-13 09:08:03 +00:00
client/accounting.go Outdated
@ -67,3 +67,1 @@
// If PrmInit.ResolveFrostFSFailures has been called, unsuccessful
// FrostFS status codes are returned as `error`, otherwise, are included
// in the returned result structure.
// If PrmInit.DontResolveFrostFSFailures has been called, unsuccessful
aarifullin commented 2023-09-13 09:08:03 +00:00
Member
Copy link

It seems PrmInit.ResolveFrostFSFailures was not correct because it referred to the inactual name.
For now it should be PrmInit.dontResolveFrostFSErrors

It seems `PrmInit.ResolveFrostFSFailures` was not correct because it referred to the inactual name. For now it should be `PrmInit.dontResolveFrostFSErrors`
olefirenque commented 2023-09-13 09:19:20 +00:00
Author
Contributor
Copy link

PrmInit.ResolveFrostFSFailures referred to the function name that is used to set the value of PrmInit.resolveFrostFSErrors field. But maybe it's better to unify the field name and the function name?

`PrmInit.ResolveFrostFSFailures` referred to the function name that is used to set the value of `PrmInit.resolveFrostFSErrors` field. But maybe it's better to unify the field name and the function name?
aarifullin commented 2023-09-15 12:14:20 +00:00
Member
Copy link

referred to the function name

Okay. That's fine then. No need to unify names :)

> referred to the function name Okay. That's fine then. No need to unify names :)
olefirenque marked this conversation as resolved
olefirenque changed title from WIP: [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option to [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option 2023-09-15 15:19:11 +00:00
olefirenque force-pushed 131-refactor_frostfs_failure_resolving_option from c133e20009 to 15bdb42d73 2023-09-15 15:21:59 +00:00 Compare
olefirenque force-pushed 131-refactor_frostfs_failure_resolving_option from 15bdb42d73 to 9d1375b453 2023-09-15 15:25:40 +00:00 Compare
olefirenque changed title from [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option to WIP: [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option 2023-09-15 15:42:29 +00:00
olefirenque force-pushed 131-refactor_frostfs_failure_resolving_option from 9d1375b453 to d241aa92bf 2023-09-15 15:52:46 +00:00 Compare
olefirenque changed title from WIP: [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option to [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option 2023-09-15 15:58:20 +00:00
fyrchik requested changes 2023-09-15 16:54:25 +00:00
fyrchik left a comment
Owner
Copy link

Good job!

Good job!
README.md Outdated
@ -43,3 +43,3 @@
var prmInit client.PrmInit
prmInit.SetDefaultPrivateKey(key) // private key for request signing
prmInit.ResolveFrostFSFailures() // enable erroneous status parsing
prmInit.DisableFrostFSFailuresResolution() // disable erroneous status parsing
fyrchik commented 2023-09-15 16:51:15 +00:00
Owner
Copy link

Can we just remove this line from doc? We would like to get rid of this in future and ResolveFrostFSFAilures is now a default behaviour, which everyone should use.

Can we just remove this line from doc? We would like to get rid of this in future and `ResolveFrostFSFAilures` is now a default behaviour, which everyone should use.
client/client.go
@ -170,0 +165,4 @@
// FrostFS protocol only in resulting structure (see corresponding Res* docs).
// These errors are returned from each protocol operation. By default, statuses
// are resolved and returned as a Go built-in errors.
func (x *PrmInit) DisableFrostFSFailuresResolution() {
fyrchik commented 2023-09-15 16:52:41 +00:00
Owner
Copy link

This will break lots of client code, can we leave this method, mark it as Deprecated and add a new one?

This will break lots of client code, can we leave this method, mark it as `Deprecated` and add a new one?
olefirenque commented 2023-09-15 18:18:56 +00:00
Author
Contributor
Copy link

PrmInit.ResolveFrostFSFailures() must be a Deprecated one without any effects and PrmInit.DisableFrostFSFailuresResolution() is a new one, right?

`PrmInit.ResolveFrostFSFailures()` must be a `Deprecated` one without any effects and `PrmInit.DisableFrostFSFailuresResolution()` is a new one, right?
client/netmap_test.go Outdated
@ -84,6 +84,7 @@ func TestClient_NetMapSnapshot(t *testing.T) {
srv.signResponse = true
// status failure
c.prm.DisableFrostFSFailuresResolution()
fyrchik commented 2023-09-15 16:54:02 +00:00
Owner
Copy link

It is better to do this near the client initialization at line 69. This way all statements in test will use the same logic (as it was before this PR).

It is better to do this near the client initialization at line 69. This way all statements in test will use the same logic (as it was before this PR).
👍 1
fyrchik requested review from storage-core-committers 2023-09-15 16:54:37 +00:00
fyrchik requested review from storage-core-developers 2023-09-15 16:54:38 +00:00
fyrchik requested review from storage-services-committers 2023-09-15 16:54:38 +00:00
fyrchik requested review from storage-services-developers 2023-09-15 16:54:38 +00:00
olefirenque added 1 commit 2023-09-15 18:31:16 +00:00
[#131] client: keep backwards-compatibility, update README.md, fix chore
All checks were successful
DCO / DCO (pull_request) Successful in 1m0s
Details
Tests and linters / Tests (1.19) (pull_request) Successful in 1m11s
Details
Tests and linters / Lint (pull_request) Successful in 3m49s
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 6m9s
Details
070db1d2c7 
Signed-off-by: Egor Olefirenko <egor.olefirenko892@gmail.com>
olefirenque changed title from [#131] client: Switch ResolveFrostFSFailures to DontResolveFrostFSFailures option to [#131] client: Switch ResolveFrostFSFailures to DisableFrostFSFailuresResolution option 2023-09-15 20:10:35 +00:00
olefirenque requested review from fyrchik 2023-09-18 18:26:24 +00:00
fyrchik approved these changes 2023-10-24 15:56:44 +00:00
fyrchik merged commit 670619d242 into master 2023-10-26 14:35:50 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-committers
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
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#163
No description provided.
Delete branch "olefirenque/frostfs-sdk-go:131-refactor_frostfs_failure_resolving_option"

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?