api: support iproto feature discovery

[DifferentialOrange]

api: support iproto feature discovery

Since version 2.10.0 Tarantool supports feature discovery [1]. Client
can send client protocol version and supported features and receive
server protocol version and supported features information to tune
its behavior.

After this patch, the request will be sent on dial, before
authentication is performed. Connector stores server info in connection
internals. User can also set option RequiredProtocolVersion and
RequiredProtocolFeatures to fast fail on connect if server does
not provide some expected feature, similar to net.box opts [2]. It is
not clear how connector should behave in case if client doesn't support
a protocol feature or protocol version, see [3]. For now we decided not
to check requirements on client side.

Feature check iterates over lists to check if feature is
enabled. It seems that iterating over a small list is way faster than
building a map, see [4]. Benchmark tests show that this check is rather
fast (0.5 ns for both client and server check on HP ProBook 440 G5) so
it is not necessary to cache it in any way.

Traces of IPROTO_FEATURE_GRACEFUL_SHUTDOWN flag and protocol version 4
could be found in Tarantool source code but they were removed in the
following commits before the release and treated like they never
existed. We also ignore them here too. See [5] for more info. In latest
master commit new feature with code 4 and protocol version 4 were
introduced [6].

1. Add iproto request to query server features tarantool#6253
2. https://www.tarantool.io/en/doc/latest/reference/reference_lua/net_box/#lua-function.net_box.new
3. Asymmetric behaviour on unknown client protocol info tarantool#7953
4. https://stackoverflow.com/a/52710077/11646599
5. Drop graceful shutdown feature flag tarantool-python#262
6. tarantool/tarantool@948e5cd

Closes #120

[oleg-jukovec]

After this patch, the request will be send on Connect.

1. What about reconnect?
2. A connection is not necessarily in the connected state after Connect(), see v2: restart on connection create #136

I think a better place will be just before auth request:

https://github.com/tarantool/tarantool/blob/bab289cd88a7f76bd1334853063066c43591fabe/src/box/lua/net_box.c#L2634-L2635

(for now client does not support any features from the list)

I guess it supports IPROTO_FEATURE_STREAMS and IPROTO_FEATURE_TRANSACTIONS at now.

In this case, protocol version would be ProtocolVersionUnsupported (-1) and no features would be enabled.

We are changing the type here from uint to int. Could we use 0 instead of -1?

https://github.com/tarantool/tarantool/blob/bab289cd88a7f76bd1334853063066c43591fabe/src/box/lua/net_box.c#L2403-L2408

For now it doesn't seems like exposing protocol version and enabled features is useful for a client so private variables are used to store this info. Getters added in export_test for tests.

Why then add this code (feature check) at all? It looks useless.
What is the Tarantool client doing now, how is this information used in it?

See:

1. https://www.tarantool.io/ru/doc/latest/reference/reference_lua/net_box/ (required_protocol_version and required_protocol_features).
2. https://github.com/tarantool/tarantool/blob/master/src/box/lua/net_box.lua#L162-L163
3. https://github.com/tarantool/tarantool/blob/master/src/box/lua/console.lua#L852-L856

[DifferentialOrange]

Could we use 0 instead of -1?

Well, protocol version is 0 for Tarantool between "version when IPROTO_ID was introduced" and "somewhere between 2.10.0-beta1 and 2.10.0-beta2 when first feature flag was added", so "unknown version" and "version 0" defines different version of Tarantool. On the other hand, it's not like we would treat -1 and 0 protocol version different in out code.

[DifferentialOrange]

I guess it supports IPROTO_FEATURE_STREAMS and IPROTO_FEATURE_TRANSACTIONS at now.

It's an interesting point. I have thought that features won't work until you enable them with feature flag. It is the case for IPROTO_FEATURE_ERROR_EXTENSION: until you send the flag, you will receive MP_ERROR objects as plain error Lua tables. But it seems that transactions and streams will work disregarding feature flag.

[DifferentialOrange]

Why then add this code (feature check) at all? t looks useless.
What is the Tarantool client doing now, how is this information used in it?

For example, it enables full MP_ERROR support, see #226 (comment)

[DifferentialOrange]

the type here from uint to int

Btw, what type should be used to store values like this? uint8, uint16, uint32, uint64, uint? Some int value? Is there any general practice here?

[oleg-jukovec]

Well, protocol version is 0 for Tarantool between "version when IPROTO_ID was introduced" and "somewhere between 2.10.0-beta1 and 2.10.0-beta2 when first feature flag was added", so "unknown version" and "version 0" defines different version of Tarantool. On the other hand, it's not like we would treat -1 and 0 protocol version different in out code.

No, it is not true, see:

tarantool/tarantool@0be1faf1fe

It was introduced with 1.

[oleg-jukovec]

the type here from uint to int

Btw, what type should be used to store values like this? uint8, uint16, uint32, uint64, uint? Some int value? Is there any general practice here?

We can hide the implementation details here:

type ProtocolVersion uint64

[oleg-jukovec]

Why then add this code (feature check) at all? t looks useless.
What is the Tarantool client doing now, how is this information used in it?

For example, it enables full MP_ERROR support, see #226 (comment)

It looks unexpectedly becase MP_ERROR has been introduced before IPROTO_ID:

tarantool/tarantool@342f601

In any case, in practice there are more uses in Tarantool (I've updated the source comment).

[Totktonada]

One of ideas behind the feature discovery functionality is to allow a user to require a feature/a protocol version at connection, so the connection will fail if the server doesn't support it. A kind of fail early approach.

[DifferentialOrange]

It looks unexpectedly becase MP_ERROR has been introduced before IPROTO_ID

It seems there were other handles like msgpack.cfg.encode_error_as_ext to control MP_ERROR support. (But this approach was rather complicated and I failed to create a connection supporting MP_ERROR for pre-2.10.0 Tarantool.)

[DifferentialOrange]

1. What about reconnect?

Added call to reconnect, reworked code in Connect to be similar to schema load one. I decided not to put feature load code to a single place in createConnection since it seems out of place.

2. A connection is not necessarily in the connected state after Connect(), see

To be honest, I don't know if I should do anything with this.

[DifferentialOrange]

One more point: I've tried to add fast fail conditions in code, but the initial attempt didn't worked out well. The idea was to "return an error if user tries to start using a stream/open a transaction if connection does not support it". But the only way to work with stream in Go code is to allocate NewStream which is a simple object allocation without any network requests. And the only way to work with streams is to send a transaction requests through Do method, so to check a transaction feature support I must place a check into stream Do depending on request encoded type. Both felt really weird to me so for now I've just added an example about how user can ensure that everything is supported.

[oleg-jukovec]

The idea was to "return an error if user tries to start using a stream/open a transaction if connection does not support it".

I have not noticed such limitations in the Tarantool implementation. For my taste, you don't have to do this.

But do not to connect to a server without some features looks potentially useful:
#226 (comment)

But it may not be possible to implement this in the current reconnection implementation.

[oleg-jukovec]

1. What about reconnect?

Added call to reconnect, reworked code in Connect to be similar to schema load one. I decided not to put feature load code to a single place in createConnection since it seems out of place.

1. Schema load is not required.
2. It's a bit more logical to check the protocol and its version first, before any request (authorization). Especially if we implement such restrictions: api: support iproto feature discovery #226 (comment)

2. A connection is not necessarily in the connected state after Connect(), see

To be honest, I don't know if I should do anything with this.

Of course, because otherwise we will have errors in this case:

#136

[oleg-jukovec]

I'm ready to merge if you resolve with @AnaNek all conversations.

[DifferentialOrange]

I'm ready to merge if you resolve with @AnaNek all conversations.

Conversations are resolved now.

[oleg-jukovec]

I'm ready to merge if you resolve with @AnaNek all conversations.

Conversations are resolved now.

 internal: copy opts on Connect

The copy is not an honest deepcopy because, for example, copying logger
or channel will break the logic.

It seems better to rename Copy() -> Clone() in the commit instead of a next one.

[DifferentialOrange]

I'm ready to merge if you resolve with @AnaNek all conversations.

Conversations are resolved now.

 internal: copy opts on Connect

The copy is not an honest deepcopy because, for example, copying logger
or channel will break the logic.

It seems better to rename Copy() -> Clone() in the commit instead of a next one.

Sorry, I missed it. Fixed

[oleg-jukovec]

Thank you!