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

fifo: fix data race for put()/take() with vinyl #64

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

oleg-jukovec
Copy link
Contributor

@oleg-jukovec oleg-jukovec commented Aug 9, 2023

put()/take() contains two parts:

  1. Get a tuple from an index.
  2. Insert a next tuple (with id based on the data from the first call)/update the current one.

For the vinyl engine it could lead to yield on the second step. As result we could put a tuple with the same id or got the same task multiple times.

This is a potential problem. It is very unlikely to get yield on the second place because changes occur in the same place in memory. So it is should not to be flushed from memory to disk right after getting a tuple. But it could happen on practice, see a problem with auto increment and concurrent lookups [1].

This issue has been fixed before for fifottl driver [2][3]. Here I tried to fix it in a similar code style to make it easier to maintain the code.

  1. auto_increment needs a persistent sequence tarantool#389
  2. Atomic take #28
  3. Atomic PUT in fifottl driver #30

drivers: fix concurrency with mvcc

By default idx:max() or idx:min() have read confirmed isolation level.
It could lead to a task duplication or double task take when we
already insert or update a task, commited, but it is not yet
confirmed.

See also:

  1. IDs of task can duplicated when mvсс is on queue#207
  2. driver: fix duplicate id error with mvvc on queue#211

@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Pull Request Test Coverage Report for Build 6096673092

  • 55 of 58 (94.83%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 91.377%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sharded_queue/utils.lua 7 10 70.0%
Totals Coverage Status
Change from base Build 6096188907: -0.1%
Covered Lines: 869
Relevant Lines: 951

💛 - Coveralls

@oleg-jukovec oleg-jukovec force-pushed the oleg-jukovec/gh-no-fifo-fix-vinyl branch 2 times, most recently from 12c6414 to 2289364 Compare August 10, 2023 13:09
@oleg-jukovec oleg-jukovec force-pushed the oleg-jukovec/gh-no-fifo-fix-vinyl branch from 2289364 to 170a299 Compare September 6, 2023 11:51
put()/take() contains two parts:

1. Get a tuple from an index.
2. Insert a next tuple (with id based on the data from the first
   call)/update the current one.

For the vinyl engine it could lead to yield on the second step. As
result we could put a tuple with the same id or got the same task
multiple times.

This is a potential problem. It is very unlikely to get yield
on the second place because changes occur in the same place
in memory. So it is should not to be flushed from memory to disk
right after getting a tuple. But it could happen on practice, see a
problem with auto increment and concurrent lookups [1].

This issue has been fixed before for fifottl driver [2][3]. Here I
tried to fix it in a similar code style to make it easier to
maintain the code.

1. tarantool/tarantool#389
2. #28
3. #30
By default idx:max() or idx:min() have read confirmed isolation level.
It could lead to a task duplication or double task take when we
already insert or update a task, commited, but it is not yet
confirmed.

See also:

1. tarantool/queue#207
2. tarantool/queue#211
@oleg-jukovec oleg-jukovec force-pushed the oleg-jukovec/gh-no-fifo-fix-vinyl branch from 170a299 to c5cfb9b Compare September 25, 2023 13:47
@oleg-jukovec oleg-jukovec merged commit 38921d5 into master Oct 31, 2023
6 checks passed
@oleg-jukovec oleg-jukovec deleted the oleg-jukovec/gh-no-fifo-fix-vinyl branch March 22, 2024 10:08
oleg-jukovec added a commit that referenced this pull request Apr 16, 2024
Overview

    The release introduces roles for Tarantool 3 and improves the module
    metrics.

Breaking changes

    Metric `sharded_queue_calls` renamed to
    `tnt_sharded_queue_api_statistics_calls_total` (#71). The metric
    now has labels in the format
    `{name = "tube_name", state = "call_type"}` instead of
    `{name = "tube_name", status = "call_type"}`.

    Metric `sharded_queue_tasks` renamed to
    `tnt_sharded_queue_api_statistics_tasks` (#71). The metric now
    has labels in the format `{name = "tube_name", state = "task_state"}`
    instead of `{name = "tube_name", status = "task_state"}`.

    The dependency `cartridge` is removed from the `rockspec` since
    the module does not require it to work with Tarantool 3 (#68).

New features

    Role `roles.sharded-queue-router` for Tarantool 3 (#68).

    Role `roles.sharded-queue-storage` for Tarantool 3 (#68).

    Metric `tnt_sharded_queue_api_role_stats` is a summary with
    quantiles of `sharded_queue.api` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_role_stats` is a summary with
    quantiles of `sharded_queue.storage` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_statistics_calls_total` as
    an equivalent of `tnt_sharded_queue_api_statistics_calls_total`
    for the `sharded_queue.storage` role.
    Values have the same meaning as the `queue` statistics `calls`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "call_type"}`

    Metric `tnt_sharded_queue_storage_statistics_tasks` as an
    equivalent of `tnt_sharded_queue_api_statistics_tasks` for
    the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `tasks`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "task_state"}`

Bugfixes

    Data race with fifo driver for put()/take() methods with vinyl
    engine (#64).
@oleg-jukovec oleg-jukovec mentioned this pull request Apr 16, 2024
oleg-jukovec added a commit that referenced this pull request Apr 16, 2024
Overview

    The release introduces roles for Tarantool 3 and improves the module
    metrics.

Breaking changes

    Metric `sharded_queue_calls` renamed to
    `tnt_sharded_queue_api_statistics_calls_total` (#71). The metric
    now has labels in the format
    `{name = "tube_name", state = "call_type"}` instead of
    `{name = "tube_name", status = "call_type"}`.

    Metric `sharded_queue_tasks` renamed to
    `tnt_sharded_queue_api_statistics_tasks` (#71). The metric now
    has labels in the format `{name = "tube_name", state = "task_state"}`
    instead of `{name = "tube_name", status = "task_state"}`.

    The dependency `cartridge` is removed from the `rockspec` since
    the module does not require it to work with Tarantool 3 (#68).

New features

    Role `roles.sharded-queue-router` for Tarantool 3 (#68).

    Role `roles.sharded-queue-storage` for Tarantool 3 (#68).

    Metric `tnt_sharded_queue_api_role_stats` is a summary with
    quantiles of `sharded_queue.api` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_role_stats` is a summary with
    quantiles of `sharded_queue.storage` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_statistics_calls_total` as
    an equivalent of `tnt_sharded_queue_api_statistics_calls_total`
    for the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `calls`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "call_type"}`

    Metric `tnt_sharded_queue_storage_statistics_tasks` as an
    equivalent of `tnt_sharded_queue_api_statistics_tasks` for
    the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `tasks`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "task_state"}`

Bugfixes

    Data race with fifo driver for put()/take() methods with vinyl
    engine (#64).
oleg-jukovec added a commit that referenced this pull request Apr 16, 2024
Overview

    The release introduces roles for Tarantool 3 and improves the module
    metrics.

Breaking changes

    Metric `sharded_queue_calls` renamed to
    `tnt_sharded_queue_api_statistics_calls_total` (#71). The metric
    now has labels in the format
    `{name = "tube_name", state = "call_type"}` instead of
    `{name = "tube_name", status = "call_type"}`.

    Metric `sharded_queue_tasks` renamed to
    `tnt_sharded_queue_api_statistics_tasks` (#71). The metric now
    has labels in the format `{name = "tube_name", state = "task_state"}`
    instead of `{name = "tube_name", status = "task_state"}`.

    The dependency `cartridge` is removed from the `rockspec` since
    the module does not require it to work with Tarantool 3 (#68).

New features

    Role `roles.sharded-queue-router` for Tarantool 3 (#68).

    Role `roles.sharded-queue-storage` for Tarantool 3 (#68).

    Metric `tnt_sharded_queue_api_role_stats` is a summary with
    quantiles of `sharded_queue.api` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_role_stats` is a summary with
    quantiles of `sharded_queue.storage` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_statistics_calls_total` as
    an equivalent of `tnt_sharded_queue_api_statistics_calls_total`
    for the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `calls`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "call_type"}`

    Metric `tnt_sharded_queue_storage_statistics_tasks` as an
    equivalent of `tnt_sharded_queue_api_statistics_tasks` for
    the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `tasks`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "task_state"}`

Bugfixes

    Data race with fifo driver for put()/take() methods with vinyl
    engine (#64).
oleg-jukovec added a commit that referenced this pull request Apr 16, 2024
Overview

    The release introduces roles for Tarantool 3 and improves the module
    metrics.

Breaking changes

    Metric `sharded_queue_calls` renamed to
    `tnt_sharded_queue_api_statistics_calls_total` (#71). The metric
    now has labels in the format
    `{name = "tube_name", state = "call_type"}` instead of
    `{name = "tube_name", status = "call_type"}`.

    Metric `sharded_queue_tasks` renamed to
    `tnt_sharded_queue_api_statistics_tasks` (#71). The metric now
    has labels in the format `{name = "tube_name", state = "task_state"}`
    instead of `{name = "tube_name", status = "task_state"}`.

    The dependency `cartridge` is removed from the `rockspec` since
    the module does not require it to work with Tarantool 3 (#68).

New features

    Role `roles.sharded-queue-router` for Tarantool 3 (#68).

    Role `roles.sharded-queue-storage` for Tarantool 3 (#68).

    Metric `tnt_sharded_queue_api_role_stats` is a summary with
    quantiles of `sharded_queue.api` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_role_stats` is a summary with
    quantiles of `sharded_queue.storage` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_statistics_calls_total` as
    an equivalent of `tnt_sharded_queue_api_statistics_calls_total`
    for the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `calls`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "call_type"}`

    Metric `tnt_sharded_queue_storage_statistics_tasks` as an
    equivalent of `tnt_sharded_queue_api_statistics_tasks` for
    the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `tasks`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "task_state"}`

Bugfixes

    Data race with fifo driver for put()/take() methods with vinyl
    engine (#64).
oleg-jukovec added a commit that referenced this pull request Apr 17, 2024
Overview

    The release introduces roles for Tarantool 3 and improves the module
    metrics.

Breaking changes

    Metric `sharded_queue_calls` renamed to
    `tnt_sharded_queue_api_statistics_calls_total` (#71). The metric
    now has labels in the format
    `{name = "tube_name", state = "call_type"}` instead of
    `{name = "tube_name", status = "call_type"}`.

    Metric `sharded_queue_tasks` renamed to
    `tnt_sharded_queue_api_statistics_tasks` (#71). The metric now
    has labels in the format `{name = "tube_name", state = "task_state"}`
    instead of `{name = "tube_name", status = "task_state"}`.

    The dependency `cartridge` is removed from the `rockspec` since
    the module does not require it to work with Tarantool 3 (#68).

New features

    Role `roles.sharded-queue-router` for Tarantool 3 (#68).

    Role `roles.sharded-queue-storage` for Tarantool 3 (#68).

    Metric `tnt_sharded_queue_api_role_stats` is a summary with
    quantiles of `sharded_queue.api` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_role_stats` is a summary with
    quantiles of `sharded_queue.storage` role API calls (#71).
    The metric includes a counter of API calls and errors.
    The metric contains labels in the following format:
    `{name = "tube_name", method = "api_call_method", status = "ok" or "error"}`

    Metric `tnt_sharded_queue_storage_statistics_calls_total` as
    an equivalent of `tnt_sharded_queue_api_statistics_calls_total`
    for the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `calls`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "call_type"}`

    Metric `tnt_sharded_queue_storage_statistics_tasks` as an
    equivalent of `tnt_sharded_queue_api_statistics_tasks` for
    the `sharded_queue.storage` role (#71).
    Values have the same meaning as the `queue` statistics `tasks`
    table.
    The metric contains labels in the following format:
    `{name = "tube_name", state = "task_state"}`

Bugfixes

    Data race with fifo driver for put()/take() methods with vinyl
    engine (#64).
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.

3 participants