tarantool / queue

Create task queues, add and take jobs, monitor failed tasks
Other
237 stars 52 forks source link

Update "register_driver" #130

Closed LeonidVas closed 4 years ago

LeonidVas commented 4 years ago

Changes of patchset:

ChangeLog:

LeonidVas commented 4 years ago

Thank you!

LGTM, but minor comments:

Regarding _'refactoring: update definition of "registerdriver" method'. AFAIU the commit does not change a behaviour, but it adds a test. It is not clear on the first glance, whether something is fixed or not. I would clarify this point.

Tests were added to increase the tests coverage, so as not to break something during refactoring.

Regarding 'refactoring: move loading core drivers to init.lua'. It seems it is not just refactoring, because it actually changes a behaviour: the check re name clash against built-in drivers now occurs at require('queue') moment despite whether box is confgured or not. I don't know, how to better reflect this fact: remove 'refactoring' prefix or just note the difference in the behaviour in the commit message. I tends to interpret 'refactoring' as 'no actual behaviour changes', but maybe 'there are behaviour changes, which aren't the main reason of changes' is okay too (not sure).

'refactoring' prefix has been removed.

LeonidVas commented 4 years ago

It was here before, but was missed in some cases. I would try to give exact case(s), where user-visible behaviour is changed. Please, try to describe it in a more explicit way, but feel free to ignore this comment if the description will became vague (sometimes it is hard to give an idea in tight words).

ChangeLog updated.