Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: more informative request canceling #407

Conversation

nurzhan-saktaganov
Copy link

Log the probable reason for unexpected requestId.
Add requestId info to context done error message.

What has been done? Why? What problem is being solved?

I didn't forget about (remove if it is not applicable):

Related issues:

@nurzhan-saktaganov nurzhan-saktaganov force-pushed the nsaktaganov/more-informative-request-cancelling branch from a8898f5 to 06ed7f8 Compare September 19, 2024 17:25
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch! I would like to use a more general request ID, request id or just request instead of requestId that refers to the internal implementation.

connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
connection.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
tarantool_test.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
nurzhan-saktaganov added a commit to nurzhan-saktaganov/go-tarantool that referenced this pull request Sep 19, 2024
nurzhan-saktaganov added a commit to nurzhan-saktaganov/go-tarantool that referenced this pull request Sep 19, 2024
nurzhan-saktaganov added a commit to nurzhan-saktaganov/go-tarantool that referenced this pull request Sep 19, 2024
@nurzhan-saktaganov nurzhan-saktaganov force-pushed the nsaktaganov/more-informative-request-cancelling branch from 67d0561 to e76c795 Compare September 19, 2024 20:30
connection.go Outdated Show resolved Hide resolved
example_test.go Outdated Show resolved Hide resolved
nurzhan-saktaganov added a commit to nurzhan-saktaganov/go-tarantool that referenced this pull request Sep 19, 2024
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch! Please, squash commits into one.

@oleg-jukovec
Copy link
Collaborator

Please, fix the linter error:
https://github.com/tarantool/go-tarantool/actions/runs/10949588796/job/30417271248?pr=407

It should be enough:

$ go install golang.org/x/tools/cmd/goimports@latest
$ goimports -w .

@nurzhan-saktaganov nurzhan-saktaganov force-pushed the nsaktaganov/more-informative-request-cancelling branch from 25033b7 to 577a38c Compare September 20, 2024 10:45
@nurzhan-saktaganov
Copy link
Author

I have fixed linter error and squashed commits into single commit.

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Sep 20, 2024

I have fixed linter error and squashed commits into single commit.

Thank you. Let's wait a bit for a second reviewer and I'll merge.

@@ -47,6 +48,8 @@ type Member struct {
Val uint
}

var contextDoneErrRegexp = regexp.MustCompile(`^context is done \(request ID [0-9]+\)$`)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused by the use of regular expressions to calculate the error type. regexp is slow compared to errors.Is. Why can't we use the errors.Is construct in this case?

Copy link
Collaborator

@oleg-jukovec oleg-jukovec Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could extend the ClientError & errors.As to catch it:

go-tarantool/errors.go

Lines 25 to 30 in 0ccdee2

// ClientError is connection error produced by this client,
// i.e. connection failures or timeouts.
type ClientError struct {
Code uint32
Msg string
}

go-tarantool/errors.go

Lines 55 to 64 in 0ccdee2

// Tarantool client error codes.
const (
ErrConnectionNotReady = 0x4000 + iota
ErrConnectionClosed = 0x4000 + iota
ErrProtocolError = 0x4000 + iota
ErrTimeouted = 0x4000 + iota
ErrRateLimited = 0x4000 + iota
ErrConnectionShutdown = 0x4000 + iota
ErrIoError = 0x4000 + iota
)

Copy link
Author

@nurzhan-saktaganov nurzhan-saktaganov Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's tests, there is no performance issue is questioned. So using regexp is ok here.
Returning here a ClientError or something else changes API.
I'm not intended to change the existing API, I'm intended just to make more informative the existing one (to avoid misleading and questions by users of library in the future).

Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

CHANGELOG.md Outdated Show resolved Hide resolved
Log the probable reason for unexpected requestId.
Add requestId info to context done error message.
@nurzhan-saktaganov nurzhan-saktaganov force-pushed the nsaktaganov/more-informative-request-cancelling branch from 577a38c to d42b1b8 Compare September 21, 2024 13:36
@oleg-jukovec oleg-jukovec merged commit 592db69 into tarantool:master Sep 22, 2024
20 checks passed
@oleg-jukovec oleg-jukovec mentioned this pull request Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants