No more assertion on 0 connection retries #145
Conversation
| long count = TARANTOOL_G(retry_count); | ||
| /* retry_count always 1 more than count of tries, since retry count | ||
| * is number of retries*/ | ||
| long count = TARANTOOL_G(retry_count) + 1; |
There was a problem hiding this comment.
I have two points here:
- Backward compatibility. We change the sense of the option.
- 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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I pushed the alternative in PR #166. No decision what to choose yet.
There was a problem hiding this comment.
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. ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
MariaHajdic
force-pushed
the
gh-83-0-retries-assertion
branch
2 times, most recently
from
c73d3b9 to
2c69e2e
Compare
MariaHajdic
force-pushed
the
gh-83-0-retries-assertion
branch
from
f4a8f12 to
1a627c3
Compare
Totktonada
force-pushed
the
gh-83-0-retries-assertion
branch
from
1a627c3 to
73d3580
Compare
|
Rebased on top of current |
Totktonada
force-pushed
the
gh-83-0-retries-assertion
branch
from
73d3580 to
8585fd3
Compare
This commit changes meanging of the `retry_count` option: now it means amount of attempts to connect after the first one, not overall attempts count. Closes tarantool#83
Totktonada
force-pushed
the
gh-83-0-retries-assertion
branch
from
8585fd3 to
425ad79
Compare
Clarified docs about this setting. There may be a confusion whether the setting means overall amount of connection attempts or amount after the initial unsuccessful attempt. Usually a retries amount option means the latter, but here it means the former. Decided to don't change behaviour or the setting name to provide perfect backward compatibility, but give meaningful error and clarify docs. See the discussion in [1]. [1]: #145 (comment) Fixes #83
Clarified docs about this setting. There may be a confusion whether the setting means overall amount of connection attempts or amount after the initial unsuccessful attempt. Usually a retries amount option means the latter, but here it means the former. Decided to don't change behaviour or the setting name to provide perfect backward compatibility, but give meaningful error and clarify docs. See the discussion in [1]. [1]: #145 (comment) Fixes #83
|
Thank you all for the participation! I'll proceed with the less intrusive change (PR #166) for the backward compatibility sake. We can return to the question after the future 0.3.4 release (deadlines are unclear, though), but take it as a proposal to improve the existing API, not as a fix of the bug (#83). |
Clarified docs about this setting. There may be a confusion whether the setting means overall amount of connection attempts or amount after the initial unsuccessful attempt. Usually a retries amount option means the latter, but here it means the former. Decided to don't change behaviour or the setting name to provide perfect backward compatibility, but give meaningful error and clarify docs. See the discussion in [1]. [1]: #145 (comment) Fixes #83
This commit changes meanging of the
retry_countoption: now it meansamount of attempts to connect after the first one, not overall attempts
count.
Closes #83