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.

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

Release all taken tasks on start #106

Merged
merged 2 commits into from Feb 28, 2020

Conversation

LeonidVas
Copy link
Contributor

If some tasks have been taken and don't released before shutdown
of the tarantool instance (for example: tarantool instance has been killed)
such task go to 'hung' state (noone can take the task now).
So, we must release all taken tasks on start of the queue module.

Fixes #66

@LeonidVas
Copy link
Contributor Author

LeonidVas commented Jan 28, 2020

Test fails (deploy fails) are not related with changes

Copy link
Contributor

@olegrok olegrok left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@Mons Mons left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

See comments below.

queue/abstract.lua Outdated Show resolved Hide resolved
queue/abstract.lua Outdated Show resolved Hide resolved
@LeonidVas LeonidVas force-pushed the lvasiliev/gh-66-release-taken-tasks-after-reboot branch from 33721d0 to 94c0997 Compare February 27, 2020 15:33
@Totktonada
Copy link
Member

Replace exit code in case of error from 255 to 1

Let's add 'test' prefix.

@Totktonada
Copy link
Member

I put two 'FIXUP' commits.

The first one squashed duplicated testing code. It also adds the test for limfifottl.

The second one adds more logs and checking of tarantool version (as was proposed before). It also fixes the potential problem: tube = recreate_tube(tube_tuple) rewrotes upvalue tube (moduel level variable) — added local. Removed forgotten space_name variable. Moved the releasing logic into a function (the reason is mostly to skip it easily by a condition, in light of #85).

After the fixups I'm happy with the change. If you're too, please, squash the fixups into the base commit and I'll proceed with the patchset.

queue/abstract.lua Show resolved Hide resolved
queue/abstract.lua Show resolved Hide resolved
@LeonidVas
Copy link
Contributor Author

About common tests. I think about it, but I'm disagree with proposed concept. If look at drivers tests it have many copy-paste code. My point of view: "We must to have some common tests which will be run for all/some drivers from list". And this is not related to the patch.
limfifottl don't need the test, because it is a wrap around fifottl.

@Totktonada
Copy link
Member

We must to have some common tests which will be run for all/some drivers from list

The test case should be either referenced from a certail test file (like 020-fifottl.t) or it should be obvious from names that certain part of cases is in a separate file (say, ddd-driver_api.t).

We discussed it more with Leonid and he want to keep current way for now and make deduplication separately. I would do it like proposed above, but this way is okay too.

limfifottl don't need the test, because it is a wrap around fifottl.

It is implementation detail, but we test public API of a driver. So I would left the test here.

@LeonidVas LeonidVas force-pushed the lvasiliev/gh-66-release-taken-tasks-after-reboot branch from b17a38b to ff1e9c9 Compare February 28, 2020 10:57
All taken tasks will be released after the server restart

If some tasks have been taken and don't released before shutdown
of the tarantool instance (for example: tarantool instance has been killed)
such task go to 'hung' state (noone can take the task now).
So, we must release all taken tasks on start of the queue module.

Fixes #66
@LeonidVas LeonidVas force-pushed the lvasiliev/gh-66-release-taken-tasks-after-reboot branch 2 times, most recently from 9e5cdae to 0bfc6c5 Compare February 28, 2020 10:59
@LeonidVas
Copy link
Contributor Author

Updated

@LeonidVas
Copy link
Contributor Author

limfifottl does not duplicate fifottl tests. So, let's continue to go that way

@LeonidVas LeonidVas force-pushed the lvasiliev/gh-66-release-taken-tasks-after-reboot branch from 0bfc6c5 to 471cc03 Compare February 28, 2020 12:12
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

LGTM.

README.md Show resolved Hide resolved
@Totktonada Totktonada merged commit 3ad7ab3 into master Feb 28, 2020
@Totktonada Totktonada deleted the lvasiliev/gh-66-release-taken-tasks-after-reboot branch February 28, 2020 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Taken task is not released back to 'ready' after Tarantool reboot
5 participants