Support call 17

[LeonidVas]

* Added support for a new binary protocol command for "call" (used
  since tarantool 1.7.2). Unlike the old version, returned data is
  not converted to tuples (like in case of using "eval").
  Default - call_16 mode.
* Added options to "call" method. It can be used to force
  call_16 / call_17 mode.

In some places, the tarantool code style has been broken to better match the file code style.

Closes #101

ChangeLog: added support of a new binary protocol command for "call"(call_17 mode since tarantool 1.7.2, returned data has an arbitrary structure).

[rybakit]

I have big doubts about the chosen API. The new method allows to mutate the internal state of an already instantiated Tarantool object, which will make the API a nightmare to work with. Imagine, that a method requires a Tarantool object as an argument, how it's supposed to know what type of call this object is configured for? What if somewhere deep in your code some sneaky method changes the call type, how much time will you spend on debugging your code before you realize the cause of the problem? How will the code look like if you need to make new and old types of calls in the same script, will you suggest remembering the current mode in a variable, changing the mode, making a call and restoring it after the call? IMHO, methods that change an object behavior after its creation are a no-go for any modern API and I believe this is the wrong direction to move forward. Please, reconsider your choice.

Btw, I raised the same questions about the global ini settings in this issue (which is still open).

[Totktonada]

@rybakit Are you have an API proposal in the mind that we can discuss? In an offline discussion @LeonidVas said that he doesn't want to obligate a user to pass all constructor arguments (possibly null) to choose call_17.

I looked at the links from #75 and, yep, we can provide an alternative way to configure settings: accept a map (array) of options instead of positional arguments in the constructor. It should be carefully designed: say, we should decide whether it will be one map or two separate ones: for uri options and client options. Should we accept an array only as the first arguments or an array in any position should be considered as the option map.

So if you have thoughts how to better do it, please, share it with us.

[LeonidVas]

I have big doubts about the chosen API. The new method allows to mutate the internal state of an already instantiated Tarantool object, which will make the API a nightmare to work with. Imagine, that a method requires a Tarantool object as an argument, how it's supposed to know what type of call this object is configured for? What if somewhere deep in your code some sneaky method changes the call type, how much time will you spend on debugging your code before you realize the cause of the problem? How will the code look like if you need to make new and old types of calls in the same script, will you suggest remembering the current mode in a variable, changing the mode, making a call and restoring it after the call? IMHO, methods that change an object behavior after its creation are a no-go for any modern API and I believe this is the wrong direction to move forward. Please, reconsider your choice.

Btw, I raised the same questions about the global ini settings in this issue (which is still open).

My point:
I want to be able to use both variant in one session. I originally suggested using 2 methods (call and call_16) to do this, but this suggestion was rejected. Then I suggested to use a setter. This is necessary so that without touching the old code, I could write a new one using new features.
I agree with your point. I need to think about how to make it better.

[rybakit]

@LeonidVas

I want to be able to use both variant in one session. I originally suggested using 2 methods (call and call_16) to do this, but this suggestion was rejected. Then I suggested to use a setter. This is necessary so that without touching the old code, I could write a new one using new features.

This sounds reasonable in theory but I think you overestimate the usefulness of this feature in practice. Tarantool 1.6 is EOLed long ago and by introducing this new method you'll just increase the migration cost for the current and even future users of this connector. It's better to be a bit less flexible now regarding this questionable feature, rather than continuing to increase the technical debt for no reason. And we are not talking about days of refactoring here, updating call16 to call17 is so minor change that it will take less than an hour to fix even a very large codebase. If users are not ready to migrate to the new version of the connector, the old one will still remain, it will not suddenly disappear with a new release :) For my connector, I dropped call16 3 years ago (!), and in all this time I only had a question about it once, and the guy was happy to use the old version of the library until they migrate to a newer tarantol/php stack. Moreover, it's not clear what is the release policy for this connector, is it stable already and no BC breaks are allowed or does the major 0 mean what it should and it's still an alpha? If latter, then there is no reason to keep supporting both call16 and call17. Also, adding a new setter method to enable a new feature is not a scalable approach, what if you decide to add extended error support, will you introduce a new set_modern_error_mode method?

@Totktonada

Are you have an API proposal in the mind that we can discuss?

I'm in favor of an associative array (map) of the configuration options as a constructor argument. It will keep object immutable, solve the other issue with the global ini settings, and make it easier to extend the connector in the future (e.g. by adding support for the new error format). Don't know if it needs to be spitted or not, I didn't dive that deep, maybe ask real users of this connector?

P.S.

Should we accept an array only as the first arguments or an array in any position should be considered as the option map.

Please keep the argument position consistent, no magic with "any argument at any position", it will only confuse users. If it will help you, in my connector I use "named constructors" (static factory methods) along with the regular constructor, so users are free to choose how to instantiate the client - create it via new Client(), or from an array of options via Client::fromOptions(), or from a dsn string via Client::fromDsn(), etc.

[Totktonada]

So, either mixed first argument in a constructor or a static method to construct from an option map. The latter looks more strict, I would prefer this way. Thanks!

NB: Hm, can we do something with retry_count now? Say, provide some other settings with clear meaning and mark retry_count as deprecated? Or it does not depend on the moment when we'll add the new way to configure the connector? Need to think around.

I think we can technically resolve #101 with a call() method option (add third argument, which is array of call options; just like in net.box's call). And can add Tarantool::fromOptions() in the scope of #75. Or solve everything here if time will permit. Up to Leonid.

Example of API for call() option:

$c = new Tarantool('localhost', 3301);
$c->call(foo, [1, 2, 3], array('call_16' => false));

[rybakit]

Hm, can we do something with retry_count now? Say, provide some other settings with clear meaning and mark retry_count as deprecated?

In my tarantool client the max_retries option means a number of retries for any unsuccessful operation, not only for failed connections. So, to highlight that here it's only about connections, maybe name it connection_attempts or [max_]conn_retries? Also, please deprecate all global ini settings, not only retry_count.

add third argument, which is array of call options;

If you decide to implement this, then consider just using a boolean flag:

$c->call('foo', [1, 2, 3]);        // call16
$c->call('foo', [1, 2, 3], false); // call16
$c->call('foo', [1, 2, 3], true);  // call17

Or do you think call can have more options in the future?

[Totktonada]

Or do you think call can have more options in the future?

I want to left the ability to add more, yes.

[LeonidVas]

Also, adding a new setter method to enable a new feature is not a scalable approach, what if you decide to add extended error support, will you introduce a new set_modern_error_mode method?

Hmm, yep. If we go this way, it should be something like set_session_settings() .

[LeonidVas]

Up to Leonid.

I added options to call().
Regarding others changes in the API - I prefer to do it as a separate task because it is a flame dangerous theme.

[Totktonada]

I feel myself almost comfortable with the PR. Only option name (call_16 vs call_17) disquiets me. I would discuss it (see the relevant comment below): not because your way is not right, but to deduce some general rule for myself and apply it in the future.

[rybakit]

Since you decided to add call16 support as an option argument to call(), maybe you already thought what would happen to this option if you later decided to implement Tarantool::fromOptions()? Will it become deprecated or will it override the constructor's option or something else?

[LeonidVas]

Since you decided to add call16 support as an option argument to call(), maybe you already thought what would happen to this option if you later decided to implement Tarantool::fromOptions()? Will it become deprecated or will it override the constructor's option or something else?

It will override the constructor's option (force call_16 or call_17 mode only for this call).

[Totktonada]

LGTM.

[Totktonada]

BTW, should not we update tarantool-php-stubs?

[rybakit]

@Totktonada Yep. And also consider proposing them to https://github.com/JetBrains/phpstorm-stubs.

[Totktonada]

Thanks!

Filed https://github.com/tarantool/tarantool-php-stubs/issues/2 and https://github.com/tarantool/tarantool-php-stubs/issues/3 to don't forget about this.