On Thu, 2021-09-23 at 12:40 +0100, Stephen Finucane wrote: > On Sun, 2021-09-05 at 11:34 +1000, Daniel Axtens wrote: > > Stephen Finucane <step...@that.guru> writes: > > > > > Start making series more useful by tracking whether the state is open, > > > meaning one or more patches are in a state with action_required=True, or > > > not. This means that the 'git-pw series list' command will only list > > > series that are actually actionable. > > > > > > Signed-off-by: Stephen Finucane <step...@that.guru> > > > --- > > > Open questions: > > > - Is there a more efficient way to do the migration than I've done? > > > - Should we expose an 'is_open' boolean attribute of a 'state' (open, > > > closed) attribute for the series views? > > > - Is there a race condition possible in how I've implemented the series > > > state checks? > > > > AIUI, series.is_open is a derived property (or I suppose in db terms a > > 'view') of series.patches. > > > > Is the cost of computing it 'live' so bad that we need to cache it in > > the db? (a 'materialised view'[1]) > > > > Is there any chance we could just always compute on demand? I'm trying > > to think of all the ways we expose series: > > > > - on the patch list page, we already also fetch all the patches > > > > - on the API /series/ page, we also fetch all the patches even for just > > the list view. > > > > so it seems to me we probably shouldn't suffer a performance hit for > > doing it live. > > The biggest impact I see is filtering. This is the main thing I want because > presently 'git pw series list' is pretty much useless without this. I don't > doing this dynamically would work because attempting to fetch, for example, > the > first 100 "closed" series will require loading every series and every patch. > You > couldn't just load the first 100 since they could be entirely/mostly "open", > meaning you'd need to load another 100, and another, and another, until you > get > 100 closed. This will need to be stored _somewhere_ fwict. > > > [1] > > https://www.datacamp.com/community/tutorials/materialized-views-postgresql > > and noting that materialised views are not supported natively in mysql - > > but you've basically done https://linuxhint.com/materialized-views-mysql > > with the refresh logic in the application layer rather than a stored > > procedure. > > > > > diff --git patchwork/models.py patchwork/models.py > > > index 58e4c51e..7441510b 100644 > > > --- patchwork/models.py > > > +++ patchwork/models.py > > > @@ -14,6 +14,7 @@ from django.conf import settings > > > from django.contrib.auth.models import User > > > from django.core.exceptions import ValidationError > > > from django.db import models > > > +from django.db import transaction > > > from django.urls import reverse > > > from django.utils.functional import cached_property > > > from django.core.validators import validate_unicode_slug > > > @@ -494,6 +495,19 @@ class Patch(SubmissionMixin): > > > for tag in tags: > > > self._set_tag(tag, counter[tag]) > > > > > > + def refresh_series_state(self): > > > + if not self.series: > > > + return > > > + > > > + # If any of the patches in the series is in an "action required" > > > state, > > > + # then the series is still open > > > + # TODO(stephenfin): Can this still race? > > > + with transaction.atomic(): > > > + self.series.is_open = self.series.patches.filter( > > > + state__action_required=True > > > + ).exists() > > > + self.series.save() > > > + > > > > AIUI based some experimentation and my experience dealing with our > > series race conditions in 2018, this can race and transactions won't > > save you here. The transaction will roll back all the changes if any > > statement fails (e.g. if you had an integrityerror from a foreign key), > > and other users won't see intermediate writes from your > > transaction. What it won't do is preserve the data dependency you're > > creating in the way a traditional mutex would. > > > > Basically, consider a concurrent parsemail and an API client changing a > > patch state and refreshing series state on the same series: > > > > > > | parsemail | API client | > > ---------------------------------------------------------------- > > | | series.patch[2].save() | > > | | BEGIN TRANSACTION | > > | | is_open = SELECT(...) = False | > > | series.patch[3].save() | | > > | BEGIN TRANSACTION | | > > | is_open = SELECT(...) = True | | > > | UPDATE series... | | > > | COMMIT TRANSACTION | | > > | | UPDATE series... | > > | | COMMIT TRANSACTION | > > > > both transactions can complete successfully and the end result is > > wrong. (You can play with this sort of thing with two concurrent > > dbshells both updating a commit_ref in a transaction, which I found very > > helpful in getting the details right.) > > > > Not materialising the view avoids persisting a result where we've lost > > the race. > > Ack, yeah, this makes sense. I need to investigate these materialized views > things, but assuming they're not an option with MySQL and SQLite then I guess > I'll need to find a way to lock the whole thing in a mutex.
It looks like 'select_for_update()' [1] will potentially do the trick? So long as I grab this lock before I create or update a patch, I should be okay? Will experiment. Stephen [1] https://docs.djangoproject.com/en/2.2/ref/models/querysets/#select-for-update > > Stephen > > PS: Sorry for the delay. Holidays. > > > Kind regards, > > Daniel > > > > > > > def save(self, *args, **kwargs): > > > if not hasattr(self, 'state') or not self.state: > > > self.state = get_default_initial_patch_state() > > > @@ -503,6 +517,7 @@ class Patch(SubmissionMixin): > > > > > > super(Patch, self).save(**kwargs) > > > > > > + self.refresh_series_state() > > > self.refresh_tag_counts() > > > > > > def is_editable(self, user): > > > @@ -772,6 +787,11 @@ class Series(FilenameMixin, models.Model): > > > 'by the subject prefix(es)') > > > total = models.IntegerField(help_text='Number of patches in series > > > as ' > > > 'indicated by the subject prefix(es)') > > > + is_open = models.BooleanField( > > > + default=True, > > > + null=True, > > > + help_text='Are all patches in this series resolved?', > > > + ) > > > > > > @staticmethod > > > def _format_name(obj): > > > diff --git patchwork/tests/api/test_series.py > > > patchwork/tests/api/test_series.py > > > index ef661773..830b2fdb 100644 > > > --- patchwork/tests/api/test_series.py > > > +++ patchwork/tests/api/test_series.py > > > @@ -16,6 +16,7 @@ from patchwork.tests.utils import create_patch > > > from patchwork.tests.utils import create_person > > > from patchwork.tests.utils import create_project > > > from patchwork.tests.utils import create_series > > > +from patchwork.tests.utils import create_state > > > from patchwork.tests.utils import create_user > > > > > > if settings.ENABLE_REST_API: > > > @@ -122,6 +123,57 @@ class TestSeriesAPI(utils.APITestCase): > > > 'submitter': 't...@example.org'}) > > > self.assertEqual(0, len(resp.data)) > > > > > > + def test_list_filter_is_open(self): > > > + """Filter series by status.""" > > > + project_obj = create_project(linkname='myproject') > > > + person_obj = create_person(email='t...@example.com') > > > + > > > + # create two series with a single patch in each with different > > > states > > > + # for the two patches > > > + > > > + state_a = create_state(name='New', slug='new', > > > action_required=True) > > > + series_a = create_series(project=project_obj, > > > submitter=person_obj) > > > + create_cover(series=series_a) > > > + create_patch(series=series_a, state=state_a) > > > + self.assertTrue(series_a.is_open) > > > + > > > + state_b = create_state( > > > + name='Accepted', slug='Accepted', action_required=False, > > > + ) > > > + series_b = create_series(project=project_obj, > > > submitter=person_obj) > > > + create_cover(series=series_b) > > > + create_patch(series=series_b, state=state_b) > > > + self.assertFalse(series_b.is_open) > > > + > > > + resp = self.client.get(self.api_url(), {'is_open': 'True'}) > > > + self.assertEqual([series_a.id], [x['id'] for x in resp.data]) > > > + > > > + resp = self.client.get(self.api_url(), {'is_open': 'False'}) > > > + self.assertEqual([series_b.id], [x['id'] for x in resp.data]) > > > + > > > + resp = self.client.get(self.api_url()) > > > + self.assertEqual( > > > + [series_a.id, series_b.id], [x['id'] for x in resp.data] > > > + ) > > > + > > > + @utils.store_samples('series-list-1-2') > > > + def test_list_version_1_2(self): > > > + """List patches using API v1.2. > > > + > > > + Validate that newer fields are dropped for older API versions. > > > + """ > > > + self._create_series() > > > + > > > + resp = self.client.get(self.api_url(version='1.2')) > > > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > > > + self.assertEqual(1, len(resp.data)) > > > + self.assertIn('url', resp.data[0]) > > > + self.assertIn('web_url', resp.data[0]) > > > + self.assertIn('web_url', resp.data[0]['cover_letter']) > > > + self.assertIn('mbox', resp.data[0]['cover_letter']) > > > + self.assertIn('web_url', resp.data[0]['patches'][0]) > > > + self.assertNotIn('is_open', resp.data[0]) > > > + > > > @utils.store_samples('series-list-1-0') > > > def test_list_version_1_0(self): > > > """List patches using API v1.0. > > > diff --git patchwork/tests/test_models.py patchwork/tests/test_models.py > > > new file mode 100644 > > > index 00000000..13db3330 > > > --- /dev/null > > > +++ patchwork/tests/test_models.py > > > @@ -0,0 +1,72 @@ > > > +# Patchwork - automated patch tracking system > > > +# Copyright (C) 2021 Stephen Finucane <step...@that.guru> > > > +# > > > +# SPDX-License-Identifier: GPL-2.0-or-later > > > + > > > +from django.test import TestCase > > > + > > > +from patchwork.tests.utils import create_patch > > > +from patchwork.tests.utils import create_series > > > +from patchwork.tests.utils import create_state > > > + > > > + > > > +class TestPatch(TestCase): > > > + > > > + def test_save_updates_series_state(self): > > > + state_new = create_state( > > > + name='New', > > > + slug='new', > > > + action_required=True, > > > + ) > > > + state_under_review = create_state( > > > + name='Under Review', > > > + slug='under-review', > > > + action_required=True, > > > + ) > > > + state_accepted = create_state( > > > + name='Accepted', > > > + slug='accepted', > > > + action_required=False, > > > + ) > > > + state_rejected = create_state( > > > + name='Rejected', > > > + slug='rejected', > > > + action_required=False, > > > + ) > > > + series = create_series() > > > + patches = [] > > > + > > > + # Create four patches - one in each state > > > + for state in ( > > > + state_new, state_under_review, state_accepted, > > > state_rejected, > > > + ): > > > + patches.append(create_patch(series=series, state=state)) > > > + > > > + # The patches will be in various states so the series should > > > still be > > > + # open > > > + self.assertTrue(patches[0].state.action_required) > > > + self.assertTrue(patches[1].state.action_required) > > > + self.assertFalse(patches[2].state.action_required) > > > + self.assertFalse(patches[3].state.action_required) > > > + self.assertTrue(series.is_open) > > > + > > > + # Now change the first patch to 'accepted'. The series shouldn't > > > change > > > + # state because the second one is still 'under-review' > > > + patches[0].state = state_accepted > > > + patches[0].save() > > > + self.assertFalse(patches[0].state.action_required) > > > + self.assertTrue(patches[1].state.action_required) > > > + self.assertFalse(patches[2].state.action_required) > > > + self.assertFalse(patches[3].state.action_required) > > > + self.assertTrue(series.is_open) > > > + > > > + # Now change the second patch to 'rejected'. The series should > > > finally > > > + # change state because all patches now have a state with > > > + # action_required=False > > > + patches[1].state = state_rejected > > > + patches[1].save() > > > + self.assertFalse(patches[0].state.action_required) > > > + self.assertFalse(patches[1].state.action_required) > > > + self.assertFalse(patches[2].state.action_required) > > > + self.assertFalse(patches[3].state.action_required) > > > + self.assertFalse(series.is_open) > > > -- > > > 2.31.1 > > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
