Proposal to change driver interface to context

[excavador]

Golang has standard library named "context"
https://golang.org/pkg/context/

This library widely used in golang libraries to implement

• cancellation of calculation tree
• timeout of calculation tree

Please read about context and how it used.

Golang standard libraries does not use context directly only because keep compatibility of API since golang 1.0.
But in some libraries you can find usage of context, for instance here:
https://golang.org/pkg/database/sql/#Conn.QueryContext

I propose to get rid of DeadlineIO struct and rewrite driver API to context usage.
It's more explicit and more golang-way, than actual implementation

[funny-falcon]

There is couple of questions:

1. will you agree if new version will be noticeably slower?
2. will you agree if API will be incompatible?
3. which versioning tool should be used to separate old and new API?

[excavador]

1. no, I am disagree. Why it would be slower?
2. incompatible with... what?

[funny-falcon]

1. Cause current request handling (especially timeout handling) is very tricky to be fast. Adding wait for channel close will inevitably slow that place.
   And given the way requests are performed, there is no way to cancel request sending to tarantool after it is written to outgoing buffer.
2. With current api.

And please describe, why do you think using Context may help to eliminate DeadlineIO structure? DeadlineIO is for handling networking and tarantool outage, it is not for request timeout.
Perhaps, I should made it private, so it doesn't appear in documentation.

[funny-falcon]

Context could be add to Future with some kind of api, and Future.wait may select for both ready and ctx.Done() channel. And sending request could be skipped if context already closed.

Even this simple steps are already not free. Adding context field will increase size of structure, and it will use next allocation size (now it fits into 64byte allocation, after addition it will be 96 byte allocation). And select is more expensive than simple channel read (but select could be skipped, if ctx is nil).

But this way there will be no huge change in internal structures.

Is it acceptable solution?

[excavador]

Cause current request handling (especially timeout handling) is very tricky to be fast. Adding wait for channel close will inevitably slow that place.

With context you just changed the way to implement timeout. Golang "select" from several channel is pretty fast, and I can not imagine how it can slow the driver

With current api.

Ah, yes, of course.

And please describe, why do you think using Context may help to eliminate DeadlineIO structure? DeadlineIO is for handling networking and tarantool outage, it is not for request timeout.

Because context is direct solution for all kind of timeout, does not matter network or not.
Under the hood the libraries uses context for implement timeout and deadline paradigm
https://golang.org/src/net/dial.go#L341

And sending request could be skipped if context already closed.

It is known and expected issue. Golang net/http server uses several different timeout for that (in addition to user context.
https://golang.org/src/net/http/server.go#L904

Even this simple steps are already not free. Adding context field will increase size of structure, and it will use next allocation size (now it fits into 64byte allocation, after addition it will be 96 byte allocation). And select is more expensive than simple channel read (but select could be skipped, if ctx is nil).

Typical application does NOT use a lot of connection. Typically amount of connection is about several tenners.

But this way there will be no huge change in internal structures.
Is it acceptable solution?

I am not too familiar with tarantool golang driver code. I think you will make better decision ,than I.
I just propose some alternative, widely used in golang world.

So, to summarize, why context is usefull and has liked by users...
Many golang application is just a kind of glue between user requests, spawn requests to other APIs and database(s).
Is pretty hard to implement some logic to manage timeout for this complex dependency graph.
context is pretty simple concept which provide deterministic and simple way to solve these problems.

Please take a look more attentive to context API: you have four capability:

• direct cancel of calculation tree - pretty usefull for shutdown application. In my current project the cancel root context of application used for handling SIGTERM/SIGINT signals, when I receive it I just call "cancel()" and join all children go-routines. Mostly imporant there, it's also complete the accept new connections to server and cancel any long-running queries to PostgreSQL (used in my project)
• cancel by timeout - very usefull to warrant the maximum read request / process response time
• deadline - pretty important concept for microservices and grpc.Please take a look slides 112-145
  https://www.slideshare.net/borisovalex/enabling-googley-microservices-with-http2-and-grpc
  is pretty similar to context, no?
• put value to context, pretty usefull to propagate some settings over all spawn calculation (does not matter for this ticket)

[funny-falcon]

With context you just changed the way to implement timeout. Golang "select" from several channel is pretty fast, and I can not imagine how it can slow the driver

You can not. I can. Golang "select" from several channel's is slow. Look at timeout implementation in this code. I doubt there is way to make it faster. (But it leads to restriction that all requests through single connection uses same timeout).

Because context is direct solution for all kind of timeout, does not matter network or not.
Under the hood the libraries uses context for implement timeout and deadline paradigm
https://golang.org/src/net/dial.go#L341

Got for Dial . Now show me for Write and Read.

Typical application does NOT use a lot of connection. Typically amount of connection is about several tenners.

go-tarantool connector uses single connection for single tarantool instance. It uses pipe-lining to achieve 1Mrps using single connection and 4 cores.

direct cancel of calculation tree - pretty usefull for shutdown application. In my current project the cancel root context of application used for handling SIGTERM/SIGINT signals, when I receive it I just call "cancel()" and join all children go-routines. Mostly imporant there, it's also complete the accept new connections to server and cancel any long-running queries to PostgreSQL (used in my project)

go-tarantool uses single connection, so there is no need to cancel connection establishing. And there is no way to cancel long-running query at all in tarantool. Single trouble I see is Future.wait() is not composable with select. It is easy to achieve by exposing Future.ready channel. Then you can combine it with waiting on context.Done().

cancel by timeout - very usefull to warrant the maximum read request / process response time

As I already said, to achieve performance there is no per-request timeout in this driver. So I cannot use ctx for timeout cancelling. And still, current timeout code just fill Future with error. There is no way to inform Tarantool to cancel the query.

deadline - pretty important concept for microservices and grpc.Please take a look slides 112-145
https://www.slideshare.net/borisovalex/enabling-googley-microservices-with-http2-and-grpc
is pretty similar to context, no?

Slides are great. But you should first force Tarantool to implement this concept. Then it will be possible to use it in driver. But now there is no way to cancel query that is already sent to Tarantool. And no way set deadline for request (if I didn't miss some changes in protocol).

put value to context, pretty usefull to propagate some settings over all spawn calculation (does not matter for this ticket)

If it doesn't matter for this ticket, why you mentioned it?

Look, man. You are writing in a way I'm dumb man who doesn't know anything about Context. I'm aware about Context and about it greatness.
But it is hard to be both fast and convenient. This driver is tailored for speed. There is alternative, mentioned in README, tailored for convenience https://github.com/viciious/go-tarantool .

The best I can do at the moment it is solution I suggest in my previous message. All other dreams could not be implemented without loss in performance.
If that suggestion is acceptable for you, I will implement it. If you think that is still not enough, you'd better use alternative.

[funny-falcon]

@excavador , could you look at e11b662 , please?
Does it looks like something reasonable?

[excavador]

Typically context passes to every call, because in working with a database connection you require different timeouts for different operation (one for write, another for read), or different timeouts for different reads (for instance)

If you need global cancel you may create a single context for that, then create children timeot contexts for every specific operation (instead of single context for whole connection)

Just my 2 cents based on 2+ years experience of golang development

[funny-falcon]

That is why there is fluent method WithCtx.
Intended pattern is:

   resp, err = conn.WithCtx(context).Select(...)
   resp, err = conn.WithCtx(context).Insert(...)

Isn't such fluent method enough?

[funny-falcon]

jfyi, currently it is a bad idea to mix requests with radically different timeouts in a single connection to Tarantool (regardless of connector).
Most likely it will be fixed soon (ie half-year or year).

[funny-falcon]

Just my 2 cents based on 2+ years experience of golang development

Please, I also have a huge Ego. Lets play without it, ok?

[eaglemoor]

any update?

[funny-falcon]

Not yet. There are a lot of changes.
Probably, it will be better to make other api version. So, probably I will rename master branch to v1 soon, and will start to develop v2.

[Totktonada]

I want to stop fork hell and offer both speed and convenience (or at least a choice) for users. My plan is the following:

• No breaking changes. New APIs are new APIs: don't break existing ones.
• Leave connection lifetime out of scope here.
  • Reason: in my understanding a connection is usually just present for the whole application and not opened/closed per application's request/event. IOW, I don't see a use case.
• Accept a Context in request methods and implement deadline and cancellation handling for requests.
  • Don't really cancel requests, just mark given request/response id for ignoring.
  • I would like to see an API proposal in the issue beforehand.
  • Avoid unnecessary locking as in Yura's PoC patch.
• Use provided Context only for deadline/cancellation handling, don't read anything from the Value dictionary.
  • It may be convenient to pass some parameters in theory, but I would leave it out of scope here.
• Add baseline performance test (say, for point — non-range — Select).
• Add performance test that aims to catch worst case with context and compare it with the baseline.
  • I would appreciate a help (especially from @funny-falcon) here: to be honest, I don't spot when exactly we'll get the worst case. IOW, select will always on two channels and select problems with many channels look irrelevant.
• Provide performance difference information (no context vs context) based on measurements for users: say, add a wiki page with results and interpretation and link it from README.

Corrections and suggestions are welcome. (No huge Ego and, to be honest, I'm new in Go.)