Skip to content

Conversation

@olegrok
Copy link
Contributor

@olegrok olegrok commented Jul 16, 2018

Close: #80

@opomuc opomuc self-requested a review July 16, 2018 08:27
Copy link
Contributor

@opomuc opomuc left a comment

Choose a reason for hiding this comment

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

Needs a change to utubettl as well

@Totktonada
Copy link
Contributor

@olegrok touch method need to be changed accordingly, yep?

Can you add a test case for both *ttl queue drivers (tests seems to be quite straighforward and that should not be a problem).

@olegrok
Copy link
Contributor Author

olegrok commented Jul 16, 2018

@Totktonada
No, touch works correctly. I sought correct behavior using touch before release.

I hope I can add tests a bit later

@Totktonada
Copy link
Contributor

touch has increment (delta) argument in seconds according to README and now it is just added to the ttl value of a task. release after your patch adds seconds * 10^6 to ttl value of a task. I think external API should use seconds and internal representation should use microseconds.

@olegrok
Copy link
Contributor Author

olegrok commented Jul 16, 2018

@Totktonada
I added tests

Yes, internal representation of touch uses microseconds. And touch works correctly. See line 116 in abstract.lua

@Totktonada
Copy link
Contributor

@olegrok Thanks! I got it, touch really works correctly. Now we have seconds for delay in release and microseconds for delta in touch in an API for drivers, but seconds everywhere in user-visible API. I think possible refactoring can go outside of scope on this PR.

LGTM. @olegrok, please proceed with merging it into master (squash + fast-forward / no squash + fast-forward / squash + merge commit / no squash + merge commit as you wish).

There are things I want to bring into focus as outcome:

  • Drivers API is unchanged with cost of non-uniform drivers API (sec / microsec).
  • External queue API was changed, but in the manner that the documentation already stated.
  • It seems that the change does not need to involve data migration for old tasks (because we anyway cannot extract previous delays from ttl).
@opomuc opomuc merged commit 6da1689 into tarantool:master Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants