Change `Call` format to 1.7 version #171

AnaNek · 2022-04-23T14:19:27Z

An incompatible change was introduced in
Tarantool 1.7 which was released in 2016.
This change is about a new binary protocol
command for CALL, which no more restricts
a function to returning an array of tuples
and allows returning an arbitrary MsgPack/JSON
result, including scalars, nil and void (nothing).

We should be in-line with tarantool here and
provide Call17 as just Call.
For backward compatibility in current go-tarantool
version build tag go_tarantool_call_17 have been
defined. This tag used to change the default Call
behavior from Call16 to Call17.

Closes #125

DifferentialOrange · 2022-04-25T07:02:09Z

Tarantula 1.7

Quite an unusual way to name our product

DifferentialOrange

Thank you for your patch! Everything seems okay except for some minor things I've pointed out in comments.

However, we definitely shouldn't merge it yet. This is a breaking change, not only by its concept, but also by its effect. There are a lot of Go-Tarantool applications that use Calls, including our PRs (for example, your #148). And each of them will be broken after merging this change.

So we should

implement and collect all of our planned v2-changes (see V2 proposals #65);
learn how to deal with packages major version tags.

The first task is related to our planning strategy, so I think there is no use to discuss it here. The second task requires some research, and for now I don't see an issue related to this specific task. Maybe we should solve it as a part of this PR or file a separate issue and assign someone.

CHANGELOG.md

const.go

multi/multi.go

DifferentialOrange

LGTM after minor fixes. Let's merge it when we will be ready to tag v2.

const.go

multi/multi.go

request.go

DifferentialOrange · 2022-04-26T07:11:38Z

Btw, we need to rebase this PR onto master since there are calls in connection_pool too.

Totktonada · 2022-05-19T23:27:37Z

I have several comments and thoughts.

An incompatible change was introduced in Tarantool 1.7 <...>

The protocol was extended in the backward-compatible way: CALL_16 still works and have the same request type number as in tarantool 1.6. The new CALL_17 request type was introduced with its own request type number.

The change in tarantool (1.7.1-233-gd025d102f) is actually incompatible: net.box's :call() behaviour was changed. But the protocol was changed in the compatible way.

Since we speaking here in context of a connector and the binary protocol, I found the cited words confusing. I propose to reword it to something like '<...> the new CALL_17 request was instroduced and the old CALL_16 request was deprecated'.

Tarantool 1.7 which was released in 2016

There were several 1.7 releases. 1.7.2 was released in 2016, 1.7.4 in 2017.

The change of the question lands into 1.7.2. I would say 1.7.2 everywhere instead of just 1.7, because 1.7.* releases are quite different.

BTW, the API documentation mentions 1.7.4. It looks like a mistake. The protocol change is 1.7.1-233-gd025d102f, it lands to 1.7.2.

allows returning an arbitrary MsgPack/JSON result

JSON looks irrelevant here. I see, it is from tarantool 1.7.2 release notes, but I found it confusing rather than explanatory.

We recently discussed another incompatible change with @oleg-jukovec (msgpack v5 support). Oleg proposed to use build tags to choose between msgpack v2 and msgpack v5. Here we can do the same.

I imagine it as follows. Now we have Call (as CALL_16) and Call_17. We can do the following:

Add Call_16.

Add const_call_16.go and const_call_17.go, which will look like so:

//go:build !go_tarantool_call_17
// +build !go_tarantool_call_17

package tarantool

const (
        CallRequest = 6 // CALL_16
)

and

//go:build go_tarantool_call_17
// +build go_tarantool_call_17

package tarantool

const (
        CallRequest = 10 // CALL_17
)

Drop the tag in the next major release and move the CallRequest constant
back to const.go with the new value (10).

It is the raw idea and I would like to hear more opinions.

CHANGELOG.md

Totktonada · 2022-06-17T20:39:32Z

connection.go

@@ -116,9 +116,9 @@ func (d defaultLogger) Report(event ConnLogKind, conn *Connection, v ...interfac
 //
 // ATTENTION: result argument for *Typed methods should deserialize from
 // msgpack array, cause Tarantool always returns result as an array.
-// For all space related methods and Call* (but not Call17*) methods Tarantool
+// For all space related methods and Call16* (but not Call*) methods Tarantool


Call still works as Call16 by default, so the replacement of Call17 to Call looks premature. I would use explicit Call16 and Call17 in the documentation.

Totktonada · 2022-06-17T20:41:44Z

connection_pool/connection_pool.go

@@ -266,8 +268,20 @@ func (connPool *ConnectionPool) Call(functionName string, args interface{}, user
 	return conn.Call(functionName, args)
 }

+// Call16 calls registered Tarantool function.
+// It uses request code for Tarantool 1.6, so result is converted to array of arrays.
+// Deprecated since 1.7.4.


I mentioned this above, but I'll remember just in case: the new IPROTO command (and the deprecation of the old one) was in 1.7.2, not 1.7.4. See https://github.com/tarantool/tarantool/releases/tag/1.7.2.

BTW, it also seems worthful to say 'Deprecated since tarantool 1.7.2', not just 1.7.2, because the latter could be misunderstood as the version of the connector.

oleg-jukovec

Thank you for the pull request! ILGTM after resolving the notes.

call_16_test.go

CHANGELOG.md

const_call_16.go

const_call_17.go

connection_pool/connection_pool.go

oleg-jukovec · 2022-06-20T09:34:19Z

It would be better to make the commit message wider:
https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message
Wrap the body to 72 characters or so.

Now it stands out a little among others commit messages.

An incompatible change was introduced in Tarantool 1.7 which was released in 2016. This change is about a new binary protocol command for CALL, which no more restricts a function to returning an array of tuples and allows returning an arbitrary MsgPack/JSON result, including scalars, nil and void (nothing). We should be in-line with tarantool here and provide `Call17` as just `Call`. For backward compatibility in current `go-tarantool` version build tag `go_tarantool_call_17` have been defined. This tag used to change the default `Call` behavior from `Call16` to `Call17`. Closes #125

AnaNek requested review from Totktonada and DifferentialOrange April 23, 2022 14:20

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch from 771b493 to 2c91f5e Compare April 24, 2022 21:07

DifferentialOrange reviewed Apr 25, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

const.go Outdated Show resolved Hide resolved

multi/multi.go Outdated Show resolved Hide resolved

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch from 2c91f5e to a82f715 Compare April 25, 2022 10:13

DifferentialOrange approved these changes Apr 25, 2022

View reviewed changes

oleg-jukovec reviewed Apr 26, 2022

View reviewed changes

request.go Outdated Show resolved Hide resolved

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch 2 times, most recently from 71088c2 to 61d9cf4 Compare April 27, 2022 07:37

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch 2 times, most recently from dcdcb39 to 2f37bff Compare June 17, 2022 20:30

Totktonada reviewed Jun 17, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Totktonada reviewed Jun 17, 2022

View reviewed changes

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch from 2f37bff to 3b3ad86 Compare June 17, 2022 20:45

Totktonada approved these changes Jun 17, 2022

View reviewed changes

oleg-jukovec approved these changes Jun 18, 2022

View reviewed changes

call_16_test.go Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

oleg-jukovec mentioned this pull request Jun 18, 2022

Add public API with request object types #162

Merged

oleg-jukovec reviewed Jun 19, 2022

View reviewed changes

const_call_16.go Outdated Show resolved Hide resolved

oleg-jukovec reviewed Jun 19, 2022

View reviewed changes

const_call_17.go Outdated Show resolved Hide resolved

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch from 3b3ad86 to f65ac1e Compare June 20, 2022 08:56

oleg-jukovec reviewed Jun 20, 2022

View reviewed changes

connection_pool/connection_pool.go Outdated Show resolved Hide resolved

AnaNek force-pushed the AnaNek/gh-125-rename-default-call branch from f65ac1e to 9818305 Compare June 20, 2022 10:29

oleg-jukovec merged commit 988edce into master Jun 20, 2022

oleg-jukovec deleted the AnaNek/gh-125-rename-default-call branch June 20, 2022 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `Call` format to 1.7 version #171

Change `Call` format to 1.7 version #171

AnaNek commented Apr 23, 2022 •

edited

Loading

DifferentialOrange commented Apr 25, 2022 •

edited

Loading

DifferentialOrange left a comment

DifferentialOrange left a comment •

edited

Loading

DifferentialOrange commented Apr 26, 2022

Totktonada commented May 19, 2022

Totktonada Jun 17, 2022

AnaNek Jun 20, 2022

Totktonada Jun 17, 2022

Totktonada Jun 17, 2022

AnaNek Jun 20, 2022

oleg-jukovec left a comment

oleg-jukovec commented Jun 20, 2022

Change Call format to 1.7 version #171

Change Call format to 1.7 version #171

Conversation

AnaNek commented Apr 23, 2022 • edited Loading

DifferentialOrange commented Apr 25, 2022 • edited Loading

DifferentialOrange left a comment

Choose a reason for hiding this comment

DifferentialOrange left a comment • edited Loading

Choose a reason for hiding this comment

DifferentialOrange commented Apr 26, 2022

Totktonada commented May 19, 2022

Totktonada Jun 17, 2022

Choose a reason for hiding this comment

AnaNek Jun 20, 2022

Choose a reason for hiding this comment

Totktonada Jun 17, 2022

Choose a reason for hiding this comment

Totktonada Jun 17, 2022

Choose a reason for hiding this comment

AnaNek Jun 20, 2022

Choose a reason for hiding this comment

oleg-jukovec left a comment

Choose a reason for hiding this comment

oleg-jukovec commented Jun 20, 2022

Change `Call` format to 1.7 version #171

Change `Call` format to 1.7 version #171

AnaNek commented Apr 23, 2022 •

edited

Loading

DifferentialOrange commented Apr 25, 2022 •

edited

Loading

DifferentialOrange left a comment •

edited

Loading