TrueCloudLab
/
frostfs-sdk-go
29
0
Fork 18
Code Issues 29 Pull Requests 3 Actions Releases Activity

[#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
olefirenque commented 2023-09-12 16:06:37 +00:00
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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

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

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

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
Poster
Collaborator

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

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
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 [#131] client: keep backwards-compatibility, update README.md, fix chore 
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 Label
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
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
There is no content yet.
Delete Branch "olefirenque/frostfs-sdk-go:131-refactor_frostfs_failure_resolving_option"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?