From e3c856b5eef574427533fdb682163087337b2d8c Mon Sep 17 00:00:00 2001 From: "Antoine R. Dumont (@ardumont)" Date: Wed, 9 Sep 2020 18:50:46 +0200 Subject: [PATCH] utils.split_range: Split into not overlapping ranges Existing listers use the `is_within_bound` [1] method from the base lister. This method uses inclusive boundaries in all cases. As some "range" task listers [2] [3] are using `split_range` function to create "overlapping" ranges, this can cause concurrent insert issues down the line [4]. This commit adapts the function `split_range` to make the generated ranges no longer overlap. [1] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/core/lister_base.py$194-199 [2] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/gitlab/tasks.py$37-41 [3] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/gitea/tasks.py$36-41 Related to T2577 --- swh/lister/gitea/tests/test_tasks.py | 35 ++++++++++++--------------- swh/lister/gitlab/tests/test_tasks.py | 35 ++++++++++++--------------- swh/lister/tests/test_utils.py | 7 +++++- swh/lister/utils.py | 21 +++++++++++++--- 4 files changed, 54 insertions(+), 44 deletions(-) diff --git a/swh/lister/gitea/tests/test_tasks.py b/swh/lister/gitea/tests/test_tasks.py index 5c070b5..c780510 100644 --- a/swh/lister/gitea/tests/test_tasks.py +++ b/swh/lister/gitea/tests/test_tasks.py @@ -4,9 +4,12 @@ # See top-level LICENSE file for more information from time import sleep -from celery.result import GroupResult -from unittest.mock import patch +from celery.result import GroupResult +from unittest.mock import patch, call + +from swh.lister.gitea.tasks import NBPAGES +from swh.lister.utils import split_range def test_ping(swh_scheduler_celery_app, swh_scheduler_celery_worker): @@ -57,13 +60,11 @@ def test_range(lister, swh_scheduler_celery_app, swh_scheduler_celery_worker): @patch("swh.lister.gitea.tasks.GiteaLister") def test_relister(lister, swh_scheduler_celery_app, swh_scheduler_celery_worker): + total_pages = 85 # setup the mocked GiteaLister lister.return_value = lister lister.run.return_value = None - lister.get_pages_information.return_value = (None, 85, None) - lister.db_partition_indices.return_value = [ - (i, i + 9) for i in range(0, 80, 10) - ] + [(80, 85)] + lister.get_pages_information.return_value = (None, total_pages, None) res = swh_scheduler_celery_app.send_task("swh.lister.gitea.tasks.FullGiteaRelister") assert res @@ -92,25 +93,21 @@ def test_relister(lister, swh_scheduler_celery_app, swh_scheduler_celery_worker) lister.get_pages_information.assert_called_once_with() # lister.run should have been called once per partition interval - for i in range(8): - # XXX inconsistent behavior: max_bound is EXCLUDED here + for min_bound, max_bound in split_range(total_pages, NBPAGES): assert ( - dict(min_bound=10 * i, max_bound=10 * i + 10), - ) in lister.run.call_args_list - assert (dict(min_bound=80, max_bound=85),) in lister.run.call_args_list + call(min_bound=min_bound, max_bound=max_bound) in lister.run.call_args_list + ) @patch("swh.lister.gitea.tasks.GiteaLister") def test_relister_instance( lister, swh_scheduler_celery_app, swh_scheduler_celery_worker ): + total_pages = 85 # setup the mocked GiteaLister lister.return_value = lister lister.run.return_value = None - lister.get_pages_information.return_value = (None, 85, None) - lister.db_partition_indices.return_value = [ - (i, i + 9) for i in range(0, 80, 10) - ] + [(80, 85)] + lister.get_pages_information.return_value = (None, total_pages, None) res = swh_scheduler_celery_app.send_task( "swh.lister.gitea.tasks.FullGiteaRelister", @@ -142,9 +139,7 @@ def test_relister_instance( lister.get_pages_information.assert_called_once_with() # lister.run should have been called once per partition interval - for i in range(8): - # XXX inconsistent behavior: max_bound is EXCLUDED here + for min_bound, max_bound in split_range(total_pages, NBPAGES): assert ( - dict(min_bound=10 * i, max_bound=10 * i + 10), - ) in lister.run.call_args_list - assert (dict(min_bound=80, max_bound=85),) in lister.run.call_args_list + call(min_bound=min_bound, max_bound=max_bound) in lister.run.call_args_list + ) diff --git a/swh/lister/gitlab/tests/test_tasks.py b/swh/lister/gitlab/tests/test_tasks.py index 1e7f27e..466da99 100644 --- a/swh/lister/gitlab/tests/test_tasks.py +++ b/swh/lister/gitlab/tests/test_tasks.py @@ -4,9 +4,12 @@ # See top-level LICENSE file for more information from time import sleep -from celery.result import GroupResult -from unittest.mock import patch +from celery.result import GroupResult +from unittest.mock import patch, call + +from swh.lister.gitea.tasks import NBPAGES +from swh.lister.utils import split_range def test_ping(swh_scheduler_celery_app, swh_scheduler_celery_worker): @@ -57,13 +60,11 @@ def test_range(lister, swh_scheduler_celery_app, swh_scheduler_celery_worker): @patch("swh.lister.gitlab.tasks.GitLabLister") def test_relister(lister, swh_scheduler_celery_app, swh_scheduler_celery_worker): + total_pages = 85 # setup the mocked GitlabLister lister.return_value = lister lister.run.return_value = None - lister.get_pages_information.return_value = (None, 85, None) - lister.db_partition_indices.return_value = [ - (i, i + 9) for i in range(0, 80, 10) - ] + [(80, 85)] + lister.get_pages_information.return_value = (None, total_pages, None) res = swh_scheduler_celery_app.send_task( "swh.lister.gitlab.tasks.FullGitLabRelister" @@ -94,25 +95,21 @@ def test_relister(lister, swh_scheduler_celery_app, swh_scheduler_celery_worker) lister.get_pages_information.assert_called_once_with() # lister.run should have been called once per partition interval - for i in range(8): - # XXX inconsistent behavior: max_bound is EXCLUDED here + for min_bound, max_bound in split_range(total_pages, NBPAGES): assert ( - dict(min_bound=10 * i, max_bound=10 * i + 10), - ) in lister.run.call_args_list - assert (dict(min_bound=80, max_bound=85),) in lister.run.call_args_list + call(min_bound=min_bound, max_bound=max_bound) in lister.run.call_args_list + ) @patch("swh.lister.gitlab.tasks.GitLabLister") def test_relister_instance( lister, swh_scheduler_celery_app, swh_scheduler_celery_worker ): + total_pages = 85 # setup the mocked GitlabLister lister.return_value = lister lister.run.return_value = None - lister.get_pages_information.return_value = (None, 85, None) - lister.db_partition_indices.return_value = [ - (i, i + 9) for i in range(0, 80, 10) - ] + [(80, 85)] + lister.get_pages_information.return_value = (None, total_pages, None) res = swh_scheduler_celery_app.send_task( "swh.lister.gitlab.tasks.FullGitLabRelister", @@ -144,9 +141,7 @@ def test_relister_instance( lister.get_pages_information.assert_called_once_with() # lister.run should have been called once per partition interval - for i in range(8): - # XXX inconsistent behavior: max_bound is EXCLUDED here + for min_bound, max_bound in split_range(total_pages, NBPAGES): assert ( - dict(min_bound=10 * i, max_bound=10 * i + 10), - ) in lister.run.call_args_list - assert (dict(min_bound=80, max_bound=85),) in lister.run.call_args_list + call(min_bound=min_bound, max_bound=max_bound) in lister.run.call_args_list + ) diff --git a/swh/lister/tests/test_utils.py b/swh/lister/tests/test_utils.py index f793b4c..defde1a 100644 --- a/swh/lister/tests/test_utils.py +++ b/swh/lister/tests/test_utils.py @@ -11,7 +11,12 @@ from swh.lister import utils @pytest.mark.parametrize( "total_pages,nb_pages,expected_ranges", - [(14, 5, [(0, 5), (5, 10), (10, 14)]), (19, 10, [(0, 10), (10, 19)])], + [ + (14, 5, [(0, 4), (5, 9), (10, 14)]), + (19, 10, [(0, 9), (10, 19)]), + (20, 3, [(0, 2), (3, 5), (6, 8), (9, 11), (12, 14), (15, 17), (18, 20)]), + (21, 3, [(0, 2), (3, 5), (6, 8), (9, 11), (12, 14), (15, 17), (18, 21),],), + ], ) def test_split_range(total_pages, nb_pages, expected_ranges): actual_ranges = list(utils.split_range(total_pages, nb_pages)) diff --git a/swh/lister/utils.py b/swh/lister/utils.py index 68e8b82..3576608 100644 --- a/swh/lister/utils.py +++ b/swh/lister/utils.py @@ -1,13 +1,28 @@ -# Copyright (C) 2018 the Software Heritage developers +# Copyright (C) 2018-2020 the Software Heritage developers # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information +from typing import Iterator, Tuple -def split_range(total_pages, nb_pages): + +def split_range(total_pages: int, nb_pages: int) -> Iterator[Tuple[int, int]]: + """Split `total_pages` into mostly `nb_pages` ranges. In some cases, the last range can + have one more element. + + >>> split_range(19, 10) + [(0, 9), (10, 19)] + + >>> split_range(20, 3) + [(0, 2), (3, 5), (6, 8), (9, 11), (12, 14), (15, 17), (18, 20)] + + >>> split_range(21, 3) + [(0, 2), (3, 5), (6, 8), (9, 11), (12, 14), (15, 17), (18, 21)] + + """ prev_index = None for index in range(0, total_pages, nb_pages): if index is not None and prev_index is not None: - yield prev_index, index + yield prev_index, index - 1 prev_index = index if index != total_pages: