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.

Sign up for GitHub

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

No more assertion on 0 connection retries #145

Closed
wants to merge 1 commit into from
+32 −4
Closed

No more assertion on 0 connection retries #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ Place it into project library path in your IDE.

* `tarantool.persistent` - Enable persistent connections (don't close connections between sessions) (defaults: True, **can't be changed in runtime**)
* `tarantool.timeout` - Connection timeout (defaults: 10 seconds, can be changed in runtime)
* `tarantool.retry_count` - Count of retries for connecting (defaults: 1, can be changed in runtime)
* `tarantool.retry_count` - Count of retries for connecting (defaults: 0, can be changed in runtime). 0 means do not retry in case the connection was failed the first time.
* `tarantool.retry_sleep` - Sleep between connecting retries (defaults: 0.1 second, can be changed in runtime)
* `tarantool.request_timeout` - Read/write timeout for requests (defaults: 10 second, can be changed in runtime)

Expand Down
11 changes: 8 additions & 3 deletions src/tarantool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ PHP_INI_BEGIN()
STD_PHP_INI_ENTRY("tarantool.request_timeout", "3600.0", PHP_INI_ALL,
OnUpdateReal, request_timeout, zend_tarantool_globals,
tarantool_globals)
STD_PHP_INI_ENTRY("tarantool.retry_count", "1", PHP_INI_ALL,
STD_PHP_INI_ENTRY("tarantool.retry_count", "0", PHP_INI_ALL,
OnUpdateLong, retry_count, zend_tarantool_globals,
tarantool_globals)
STD_PHP_INI_ENTRY("tarantool.retry_sleep", "10", PHP_INI_ALL,
Expand Down Expand Up @@ -252,7 +252,12 @@ static int __tarantool_connect(tarantool_object *t_obj) {
TSRMLS_FETCH();
tarantool_connection *obj = t_obj->obj;
int status = SUCCESS;
long count = TARANTOOL_G(retry_count);
/*
* Amount of connection attempts is always 1 more than
* `retry_count` option value, since the option means
* amount of attempts after the first one.
*/
long count = TARANTOOL_G(retry_count) + 1;
Copy link
Member

Choose a reason for hiding this comment

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

I have two points here:

  1. Backward compatibility. We change the sense of the option.
  2. What we want the option to mean? Tries count or tries count after the first one? I think the first (old behaviour). The real issue was the error message. I think we should get the old behaviour back and give a user an error message indicating the zero is not the valid value for that option.

Copy link
Collaborator

@rybakit rybakit Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@Totktonada I think that 0 is still a valid value for retry_count in case you want to disable the retrying functionality. But, in overall, I would completely drop this ini setting (and others as well) for the reasons I stated in #75.

Copy link
Member

Choose a reason for hiding this comment

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

@rybakit 0 is not valid 'tries count'. 0 is valid 'tries count after the first one`. The option (before this commit) means 'tries count'. So I don't get your point.

This can be alias for 1, but I think it'll just introduce more confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My point is that I would like to have a way to disable retries. I don't really care if it starts from 1 or 0 as long as it can be disabled entirely.

Copy link
Collaborator

@rybakit rybakit Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

0 is not valid 'tries count'.

Btw, I was talking about the tarantool.retry_count setting. And it's called *re*try_count, so I don't see your confusion, as I read it, it can be one insufficient call and 0 retry calls afterward.

Copy link
Member

Choose a reason for hiding this comment

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

I pushed the alternative in PR #166. No decision what to choose yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

However there are exceptions: --retries in parallel

From man parallel:

--retries n
    If a job fails, retry it on another computer on which it has not failed. Do this n times. ...

Copy link
Member

Choose a reason for hiding this comment

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

However there are exceptions: --retries in parallel

From man parallel:

--retries n
    If a job fails, retry it on another computer on which it has not failed. Do this n times. ...

The link above shows example that looks opposite.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The link above shows example that looks opposite.

I know, I just wanted to show that maybe parallel is not the best example to follow because its behavior does not reflect what is stated in its manual.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, Hi guys. IMHO @rybakit is damn right and the name of the option not match the content. But, backward compatibility is a very strong argument. I think updating the description is the optimal decision. I vote for #166 .

struct timespec sleep_time = {0};
double_to_ts(INI_FLT("retry_sleep"), &sleep_time);
char *err = NULL;
Expand Down Expand Up @@ -962,7 +967,7 @@ PHP_RINIT_FUNCTION(tarantool) {

static void php_tarantool_init_globals(zend_tarantool_globals *tarantool_globals) {
tarantool_globals->sync_counter = 0;
tarantool_globals->retry_count = 1;
tarantool_globals->retry_count = 0;
tarantool_globals->retry_sleep = 10;
tarantool_globals->timeout = 3600.0;
tarantool_globals->request_timeout = 3600.0;
Expand Down
23 changes: 23 additions & 0 deletions test/CreateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,5 +163,28 @@ public static function provideGoodCredentials()
['guest', null],
];
}

public function test_10_zero_retry_exception() {
/*
* gh-83: An attempt to connection with `retry_count = 0`
* gives the following error:
*
* | Uncaught TarantoolIOException: (null)
*
* Now the option means amount of attempts to connect
* after the first one, not an overall attempts to
* connect. So zero becomes valid value.
*/

$saved_retry_count = ini_get('tarantool.retry_count');
ini_set('tarantool.retry_count', 0);

try {
$c = new Tarantool('localhost', self::$port);
$this->assertEquals($c->ping(), true);
} finally {
ini_set('tarantool.retry_count', $saved_retry_count);
}
}
}