utubettl: remove unnecessary on_task_change call #97
Conversation
iskandarov-egor force-pushed the master branch from 80c1771 to 054b552 Compare 
LeonidVas left a comment
LeonidVas left a comment There was a problem hiding this comment.
Hi. Thank you for the patch. Sorry for the big latency.
queue/abstract/driver/utubettl.lua Outdated
| self:on_task_change(neighbour) | ||
| end | ||
| local function wake_neighbour(self, utube) | ||
| local neighbour = self.space.index.utube:select({state.READY, utube}, {limit=1})[1] |
There was a problem hiding this comment.
I think using min() with checks is more correct (with link to tarantool/tarantool#3167)
There was a problem hiding this comment.
aren't they equivalent?
There was a problem hiding this comment.
I meant that the behavior of max() was different from expected and I see no reason to replace min() to select(). I agree with you that the default iterator is ALL(same as GE) and the results will be equivalent. But generally min () is used in utubettl driver and IMHO: it better reflects the idea. I may be wrong. What is the reason for choosing select()?
There was a problem hiding this comment.
from the 1.10 docs:
In a future version of Tarantool, index:min(key value) will return nothing if key value is not equal to a value in the index.
so we cannot be sure that neighbour will be from the same utube on tarantool 1.10
There was a problem hiding this comment.
Hmm, as I understand you say about tarantool/tarantool@8bbd4a6 . Looks like you're right! Please add a comment. If you add a test, it will be great!
There was a problem hiding this comment.
I meant that the behavior of max() was different from expected and I see no reason to replace min() to select(). I agree with you that the default iterator is ALL(same as GE) and the results will be equivalent.
The default iterator is 'EQ'.
Totktonada left a comment
Totktonada left a comment There was a problem hiding this comment.
Regarding commit 'utubettl: remove unnecessary on_task_change call'. It seems it is possible that a next task of utube may be taken before the previous one is acked. It violates guarantees of strict odrdering within a utube. A test case should be added and utube queue type should be verified too (not only utubettl).
Regarding commit 'utubettl: if the task being deleted was not taken, do not wake neighbour task'. It also looks as a fix for possible incorrect behaviour (violate of ordering guarantees) and there should be a test case, which shows it. The same for utube queue type.
queue/abstract/driver/utubettl.lua Outdated
| if task[i_status] == state.TAKEN then | ||
| wake_neighbour(self, task[i_utube]) | ||
| end |
There was a problem hiding this comment.
It can be implemented in less intruisive way, without refactoring of suppounding code.
local is_taken = task[i_status] == state.TAKEN task = task:transform(i_status, 1, state.DONE) if is_taken then return process_neighbour(self, task, 'delete') else self:on_task_change(task, 'delete') return task end
How? Does my patch introduce this bug, or does it already exist? |
| I had a conversation with @Totktonada and your code is generally ok. Now it needs to be decorated in accordance with the general points https://github.com/tarantool/tarantool/wiki/Code-review-procedure (commit message, comments, tests if possible). You can do this work yourself or forward it to us. Which pill do you choose? |
I think we should make an optimistic merge in the future :) |
Only sceptics can keep quality on the production level. Sometimes you're lucky and 'rapid development' approach seems to work, but it does not scale to a time period like several years. There are ways to exploit this rule: say, hire a large QA team and perform thorough pre-release testing. But it is costly. It is based on my experience, which is not a gold standard. Maybe your experience in long living projects is different. |
| Has been updated and pushed to master. |
4 participants
No description provided.