> Closes: #241 I'm not sure this is quite correct, sadly. Using the parallel parser I posted, I see a difference in the number of series created by parsing in parallel and parsing serially.
# serial (-j1) >>> Series.objects.count() 28016 # parallel (-j6) >>> Series.objects.count() 28065 I investigated just the first Untitled series from the parallel case, where I see that one 6-patch series has been split up a lot: >>> pprint.pprint([(p.name, p.series) for p in >>> Patch.objects.filter(submitter__id=42)[0:6]]) [('[1/6] x86: implement support to synchronize RDTSC through MFENCE on AMD ' 'CPUs', <Series: [1/6] x86: implement support to synchronize RDTSC through MFENCE on AMD CPUs>), ('[2/6] x86: Implement support to synchronize RDTSC with LFENCE on Intel CPUs', <Series: Untitled series #307>), ('[3/6] x86: move nop declarations into separate include file', <Series: Untitled series #544>), ('[4/6] x86: introduce rdtsc_barrier()', <Series: Untitled series #307>), ('[5/6] x86: remove get_cycles_sync', <Series: Untitled series #559>), ('[6/6] x86: read_tsc sync', <Series: Untitled series #449>)] If I do serial parsing, this does not get split up: >>> pprint.pprint([(p.name, p.series) for p in >>> Patch.objects.filter(submitter__id=74)[0:6]]) [('[1/6] x86: implement support to synchronize RDTSC through MFENCE on AMD ' 'CPUs', <Series: Get rid of CPUID from gettimeofday>), ('[2/6] x86: Implement support to synchronize RDTSC with LFENCE on Intel CPUs', <Series: Get rid of CPUID from gettimeofday>), ('[3/6] x86: move nop declarations into separate include file', <Series: Get rid of CPUID from gettimeofday>), ('[4/6] x86: introduce rdtsc_barrier()', <Series: Get rid of CPUID from gettimeofday>), ('[5/6] x86: remove get_cycles_sync', <Series: Get rid of CPUID from gettimeofday>), ('[6/6] x86: read_tsc sync', <Series: Get rid of CPUID from gettimeofday>)] I haven't looked at the series in any detail beyond this - it might still be worth including, but it just doesn't quite squash the bug, sadly. Regards, Daniel > Cc: Daniel Axtens <d...@axtens.net> > Cc: Petr Vorel <petr.vo...@gmail.com> > --- > patchwork/migrations/0037_python_3.py | 298 ++++++++++++++++++ > .../0038_unique_series_references.py | 121 +++++++ > patchwork/models.py | 6 +- > patchwork/parser.py | 122 +++---- > patchwork/tests/test_parser.py | 9 +- > patchwork/tests/utils.py | 6 +- > 6 files changed, 496 insertions(+), 66 deletions(-) > create mode 100644 patchwork/migrations/0037_python_3.py > create mode 100644 patchwork/migrations/0038_unique_series_references.py > > diff --git patchwork/migrations/0037_python_3.py > patchwork/migrations/0037_python_3.py > new file mode 100644 > index 00000000..416a7045 > --- /dev/null > +++ patchwork/migrations/0037_python_3.py > @@ -0,0 +1,298 @@ > +import datetime > + > +from django.conf import settings > +from django.db import migrations, models > +import django.db.models.deletion > + > +import patchwork.models > + > + > +class Migration(migrations.Migration): > + > + dependencies = [('patchwork', '0036_project_commit_url_format')] > + > + operations = [ > + migrations.AlterField( > + model_name='check', > + name='context', > + field=models.SlugField( > + default='default', > + help_text='A label to discern check from checks of other > testing systems.', # noqa > + max_length=255, > + ), > + ), > + migrations.AlterField( > + model_name='check', > + name='description', > + field=models.TextField( > + blank=True, > + help_text='A brief description of the check.', > + null=True, > + ), > + ), > + migrations.AlterField( > + model_name='check', > + name='state', > + field=models.SmallIntegerField( > + choices=[ > + (0, 'pending'), > + (1, 'success'), > + (2, 'warning'), > + (3, 'fail'), > + ], > + default=0, > + help_text='The state of the check.', > + ), > + ), > + migrations.AlterField( > + model_name='check', > + name='target_url', > + field=models.URLField( > + blank=True, > + help_text='The target URL to associate with this check. This > should be specific to the patch.', # noqa > + null=True, > + ), > + ), > + migrations.AlterField( > + model_name='comment', > + name='submission', > + field=models.ForeignKey( > + on_delete=django.db.models.deletion.CASCADE, > + related_name='comments', > + related_query_name='comment', > + to='patchwork.Submission', > + ), > + ), > + migrations.AlterField( > + model_name='delegationrule', > + name='path', > + field=models.CharField( > + help_text='An fnmatch-style pattern to match filenames > against.', # noqa > + max_length=255, > + ), > + ), > + migrations.AlterField( > + model_name='delegationrule', > + name='priority', > + field=models.IntegerField( > + default=0, > + help_text='The priority of the rule. Rules with a higher > priority will override rules with lower priorities', # noqa > + ), > + ), > + migrations.AlterField( > + model_name='delegationrule', > + name='user', > + field=models.ForeignKey( > + help_text='A user to delegate the patch to.', > + on_delete=django.db.models.deletion.CASCADE, > + to=settings.AUTH_USER_MODEL, > + ), > + ), > + migrations.AlterField( > + model_name='emailconfirmation', > + name='type', > + field=models.CharField( > + choices=[ > + ('userperson', 'User-Person association'), > + ('registration', 'Registration'), > + ('optout', 'Email opt-out'), > + ], > + max_length=20, > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='category', > + field=models.CharField( > + choices=[ > + ('cover-created', 'Cover Letter Created'), > + ('patch-created', 'Patch Created'), > + ('patch-completed', 'Patch Completed'), > + ('patch-state-changed', 'Patch State Changed'), > + ('patch-delegated', 'Patch Delegate Changed'), > + ('check-created', 'Check Created'), > + ('series-created', 'Series Created'), > + ('series-completed', 'Series Completed'), > + ], > + db_index=True, > + help_text='The category of the event.', > + max_length=20, > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='cover', > + field=models.ForeignKey( > + blank=True, > + help_text='The cover letter that this event was created > for.', > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + related_name='+', > + to='patchwork.CoverLetter', > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='date', > + field=models.DateTimeField( > + default=datetime.datetime.utcnow, > + help_text='The time this event was created.', > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='patch', > + field=models.ForeignKey( > + blank=True, > + help_text='The patch that this event was created for.', > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + related_name='+', > + to='patchwork.Patch', > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='project', > + field=models.ForeignKey( > + help_text='The project that the events belongs to.', > + on_delete=django.db.models.deletion.CASCADE, > + related_name='+', > + to='patchwork.Project', > + ), > + ), > + migrations.AlterField( > + model_name='event', > + name='series', > + field=models.ForeignKey( > + blank=True, > + help_text='The series that this event was created for.', > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + related_name='+', > + to='patchwork.Series', > + ), > + ), > + migrations.AlterField( > + model_name='patch', > + name='number', > + field=models.PositiveSmallIntegerField( > + default=None, > + help_text='The number assigned to this patch in the series', > + null=True, > + ), > + ), > + migrations.AlterField( > + model_name='project', > + name='commit_url_format', > + field=models.CharField( > + blank=True, > + help_text='URL format for a particular commit. {} will be > replaced by the commit SHA.', # noqa > + max_length=2000, > + ), > + ), > + migrations.AlterField( > + model_name='project', > + name='list_archive_url_format', > + field=models.CharField( > + blank=True, > + help_text="URL format for the list archive's Message-ID > redirector. {} will be replaced by the Message-ID.", # noqa > + max_length=2000, > + ), > + ), > + migrations.AlterField( > + model_name='project', > + name='subject_match', > + field=models.CharField( > + blank=True, > + default='', > + help_text='Regex to match the subject against if only part > of emails sent to the list belongs to this project. Will be used with > IGNORECASE and MULTILINE flags. If rules for more projects match the first > one returned from DB is chosen; empty field serves as a default for every > email which has no other match.', # noqa > + max_length=64, > + validators=[patchwork.models.validate_regex_compiles], > + ), > + ), > + migrations.AlterField( > + model_name='series', > + name='name', > + field=models.CharField( > + blank=True, > + help_text='An optional name to associate with the series, > e.g. "John\'s PCI series".', # noqa > + max_length=255, > + null=True, > + ), > + ), > + migrations.AlterField( > + model_name='series', > + name='total', > + field=models.IntegerField( > + help_text='Number of patches in series as indicated by the > subject prefix(es)' # noqa > + ), > + ), > + migrations.AlterField( > + model_name='series', > + name='version', > + field=models.IntegerField( > + default=1, > + help_text='Version of series as indicated by the subject > prefix(es)', # noqa > + ), > + ), > + migrations.AlterField( > + model_name='seriesreference', > + name='series', > + field=models.ForeignKey( > + on_delete=django.db.models.deletion.CASCADE, > + related_name='references', > + related_query_name='reference', > + to='patchwork.Series', > + ), > + ), > + migrations.AlterField( > + model_name='tag', > + name='abbrev', > + field=models.CharField( > + help_text='Short (one-or-two letter) abbreviation for the > tag, used in table column headers', # noqa > + max_length=2, > + unique=True, > + ), > + ), > + migrations.AlterField( > + model_name='tag', > + name='pattern', > + field=models.CharField( > + help_text='A simple regex to match the tag in the content of > a message. Will be used with MULTILINE and IGNORECASE flags. eg. ^Acked-by:', > # noqa > + max_length=50, > + validators=[patchwork.models.validate_regex_compiles], > + ), > + ), > + migrations.AlterField( > + model_name='tag', > + name='show_column', > + field=models.BooleanField( > + default=True, > + help_text="Show a column displaying this tag's count in the > patch list view", # noqa > + ), > + ), > + migrations.AlterField( > + model_name='userprofile', > + name='items_per_page', > + field=models.PositiveIntegerField( > + default=100, help_text='Number of items to display per page' > + ), > + ), > + migrations.AlterField( > + model_name='userprofile', > + name='send_email', > + field=models.BooleanField( > + default=False, > + help_text='Selecting this option allows patchwork to send > email on your behalf', # noqa > + ), > + ), > + migrations.AlterField( > + model_name='userprofile', > + name='show_ids', > + field=models.BooleanField( > + default=False, > + help_text='Show click-to-copy patch IDs in the list view', > + ), > + ), > + ] > diff --git patchwork/migrations/0038_unique_series_references.py > patchwork/migrations/0038_unique_series_references.py > new file mode 100644 > index 00000000..c5517ada > --- /dev/null > +++ patchwork/migrations/0038_unique_series_references.py > @@ -0,0 +1,121 @@ > +from django.db import connection, migrations, models > +from django.db.models import Count > +import django.db.models.deletion > + > + > +def merge_duplicate_series(apps, schema_editor): > + SeriesReference = apps.get_model('patchwork', 'SeriesReference') > + Patch = apps.get_model('patchwork', 'Patch') > + > + msgid_seriesrefs = {} > + > + # find all SeriesReference that share a msgid but point to different > series > + # and decide which of the series is going to be the authoritative one > + msgid_counts = ( > + SeriesReference.objects.values('msgid') > + .annotate(count=Count('msgid')) > + .filter(count__gt=1) > + ) > + for msgid_count in msgid_counts: > + msgid = msgid_count['msgid'] > + chosen_ref = None > + for series_ref in SeriesReference.objects.filter(msgid=msgid): > + if series_ref.series.cover_letter: > + if chosen_ref: > + # I don't think this can happen, but explode if it does > + raise Exception( > + "Looks like you've got two or more series that share > " > + "some patches but do not share a cover letter. > Unable " > + "to auto-resolve." > + ) > + > + # if a series has a cover letter, that's the one we'll group > + # everything under > + chosen_ref = series_ref > + > + if not chosen_ref: > + # if none of the series have cover letters, simply use the last > + # one (hint: this relies on Python's weird scoping for for loops > + # where 'series_ref' is still accessible outside the loop) > + chosen_ref = series_ref > + > + msgid_seriesrefs[msgid] = chosen_ref > + > + # reassign any patches referring to non-authoritative series to point to > + # the authoritative one, and delete the other series; we do this > separately > + # to allow us a chance to raise the exception above if necessary > + for msgid, chosen_ref in msgid_seriesrefs.items(): > + for series_ref in SeriesReference.objects.filter(msgid=msgid): > + if series_ref == chosen_ref: > + continue > + > + # update the patches to point to our chosen series instead, on > the > + # assumption that all other metadata is correct > + for patch in Patch.objects.filter(series=series_ref.series): > + patch.series = chosen_ref.series > + patch.save() > + > + # delete the other series (which will delete the series ref) > + series_ref.series.delete() > + > + > +def copy_project_field(apps, schema_editor): > + if connection.vendor == 'postgresql': > + schema_editor.execute( > + """ > + UPDATE patchwork_seriesreference > + SET project_id = patchwork_series.project_id > + FROM patchwork_series > + WHERE patchwork_seriesreference.series_id = patchwork_series.id > + """ > + ) > + elif connection.vendor == 'mysql': > + schema_editor.execute( > + """ > + UPDATE patchwork_seriesreference, patchwork_series > + SET patchwork_seriesreference.project_id = > patchwork_series.project_id > + WHERE patchwork_seriesreference.series_id = patchwork_series.id > + """ > + ) # noqa > + else: > + SeriesReference = apps.get_model('patchwork', 'SeriesReference') > + > + for series_ref in SeriesReference.objects.all().select_related( > + 'series' > + ): > + series_ref.project = series_ref.series.project > + series_ref.save() > + > + > +class Migration(migrations.Migration): > + > + dependencies = [('patchwork', '0037_python_3')] > + > + operations = [ > + migrations.RunPython( > + merge_duplicate_series, migrations.RunPython.noop, atomic=False > + ), > + migrations.AddField( > + model_name='seriesreference', > + name='project', > + field=models.ForeignKey( > + null=True, > + on_delete=django.db.models.deletion.CASCADE, > + to='patchwork.Project', > + ), > + ), > + migrations.AlterUniqueTogether( > + name='seriesreference', unique_together={('project', 'msgid')} > + ), > + migrations.RunPython( > + copy_project_field, migrations.RunPython.noop, atomic=False > + ), > + migrations.AlterField( > + model_name='seriesreference', > + name='project', > + field=models.ForeignKey( > + on_delete=django.db.models.deletion.CASCADE, > + to='patchwork.Project', > + ), > + ), > + ] > diff --git patchwork/models.py patchwork/models.py > index c198bc2c..e5bfecf8 100644 > --- patchwork/models.py > +++ patchwork/models.py > @@ -79,7 +79,8 @@ class Project(models.Model): > webscm_url = models.CharField(max_length=2000, blank=True) > list_archive_url = models.CharField(max_length=2000, blank=True) > list_archive_url_format = models.CharField( > - max_length=2000, blank=True, > + max_length=2000, > + blank=True, > help_text="URL format for the list archive's Message-ID redirector. " > "{} will be replaced by the Message-ID.") > commit_url_format = models.CharField( > @@ -783,6 +784,7 @@ class SeriesReference(models.Model): > required to handle the case whereby one or more patches are > received before the cover letter. > """ > + project = models.ForeignKey(Project, on_delete=models.CASCADE) > series = models.ForeignKey(Series, related_name='references', > related_query_name='reference', > on_delete=models.CASCADE) > @@ -792,7 +794,7 @@ class SeriesReference(models.Model): > return self.msgid > > class Meta: > - unique_together = [('series', 'msgid')] > + unique_together = [('project', 'msgid')] > > > class Bundle(models.Model): > diff --git patchwork/parser.py patchwork/parser.py > index 7dc66bc0..4b837ae0 100644 > --- patchwork/parser.py > +++ patchwork/parser.py > @@ -16,6 +16,7 @@ import re > > from django.contrib.auth.models import User > from django.db.utils import IntegrityError > +from django.db import transaction > from django.utils import six > > from patchwork.models import Comment > @@ -251,16 +252,9 @@ def _find_series_by_references(project, mail): > for ref in refs: > try: > return SeriesReference.objects.get( > - msgid=ref[:255], series__project=project).series > + msgid=ref[:255], project=project).series > except SeriesReference.DoesNotExist: > continue > - except SeriesReference.MultipleObjectsReturned: > - # FIXME: Open bug: this can happen when we're processing > - # messages in parallel. Pick the first and log. > - logger.error("Multiple SeriesReferences for %s in project %s!" % > - (ref[:255], project.name)) > - return SeriesReference.objects.filter( > - msgid=ref[:255], series__project=project).first().series > > > def _find_series_by_markers(project, mail, author): > @@ -1029,47 +1023,64 @@ def parse_mail(mail, list_id=None): > except IntegrityError: > raise DuplicateMailError(msgid=msgid) > > - # if we don't have a series marker, we will never have an existing > - # series to match against. > - series = None > - if n: > - series = find_series(project, mail, author) > + for attempt in range(10): # arbitrary retry count > + try: > + with transaction.atomic(): > + # if we don't have a series marker, we will never have an > + # existing series to match against. > + series = None > + if n: > + series = find_series(project, mail, author) > + else: > + x = n = 1 > + > + # We will create a new series if: > + # - there is no existing series to assign this patch to, > or > + # - there is an existing series, but it already has a > patch > + # with this number in it > + if not series or Patch.objects.filter( > + series=series, number=x).count(): > + series = Series.objects.create( > + project=project, > + date=date, > + submitter=author, > + version=version, > + total=n) > + > + # NOTE(stephenfin) We must save references for > series. > + # We do this to handle the case where a later patch > is > + # received first. Without storing references, it > would > + # not be possible to identify the relationship > between > + # patches as the earlier patch does not reference the > + # later one. > + for ref in refs + [msgid]: > + ref = ref[:255] > + # we don't want duplicates > + try: > + # we could have a ref to a previous series. > + # (For example, a series sent in reply to > + # another series.) That should not create a > + # series ref for this series, so check for > the > + # msg-id only, not the msg-id/series pair. > + SeriesReference.objects.get( > + msgid=ref, project=project) > + except SeriesReference.DoesNotExist: > + SeriesReference.objects.create( > + msgid=ref, project=project, > series=series) > + break > + except IntegrityError: > + # we lost the race so go again > + logger.info('Conflict while saving series. This is ' > + 'probably because multiple patches belonging ' > + 'to the same series have been received at ' > + 'once. Trying again (attempt %02d/10)', > + attempt) > else: > - x = n = 1 > - > - # We will create a new series if: > - # - there is no existing series to assign this patch to, or > - # - there is an existing series, but it already has a patch with this > - # number in it > - if not series or Patch.objects.filter(series=series, > number=x).count(): > - series = Series.objects.create( > - project=project, > - date=date, > - submitter=author, > - version=version, > - total=n) > - > - # NOTE(stephenfin) We must save references for series. We > - # do this to handle the case where a later patch is > - # received first. Without storing references, it would not > - # be possible to identify the relationship between patches > - # as the earlier patch does not reference the later one. > - for ref in refs + [msgid]: > - ref = ref[:255] > - # we don't want duplicates > - try: > - # we could have a ref to a previous series. (For > - # example, a series sent in reply to another > - # series.) That should not create a series ref > - # for this series, so check for the msg-id only, > - # not the msg-id/series pair. > - SeriesReference.objects.get(msgid=ref, > - series__project=project) > - except SeriesReference.DoesNotExist: > - SeriesReference.objects.create(series=series, msgid=ref) > - except SeriesReference.MultipleObjectsReturned: > - logger.error("Multiple SeriesReferences for %s" > - " in project %s!" % (ref, project.name)) > + # we failed to save the series so return the series-less patch > + logger.warning('Failed to save series. Your patch has been ' > + 'saved but this should not happen. Please ' > + 'report this as a bug and include logs') > + return patch > > # add to a series if we have found one, and we have a numbered > # patch. Don't add unnumbered patches (for example diffs sent > @@ -1107,14 +1118,9 @@ def parse_mail(mail, list_id=None): > # message > try: > series = SeriesReference.objects.get( > - msgid=msgid, series__project=project).series > + msgid=msgid, project=project).series > except SeriesReference.DoesNotExist: > series = None > - except SeriesReference.MultipleObjectsReturned: > - logger.error("Multiple SeriesReferences for %s" > - " in project %s!" % (msgid, project.name)) > - series = SeriesReference.objects.filter( > - msgid=msgid, series__project=project).first().series > > if not series: > series = Series.objects.create( > @@ -1127,12 +1133,8 @@ def parse_mail(mail, list_id=None): > # we don't save the in-reply-to or references fields > # for a cover letter, as they can't refer to the same > # series > - try: > - SeriesReference.objects.get_or_create(series=series, > - msgid=msgid) > - except SeriesReference.MultipleObjectsReturned: > - logger.error("Multiple SeriesReferences for %s" > - " in project %s!" % (msgid, project.name)) > + SeriesReference.objects.create( > + msgid=msgid, project=project, series=series) > > try: > cover_letter = CoverLetter.objects.create( > diff --git patchwork/tests/test_parser.py patchwork/tests/test_parser.py > index e5a2fca3..a69c64ba 100644 > --- patchwork/tests/test_parser.py > +++ patchwork/tests/test_parser.py > @@ -361,17 +361,20 @@ class SeriesCorrelationTest(TestCase): > msgids = [make_msgid()] > project = create_project() > series_v1 = create_series(project=project) > - create_series_reference(msgid=msgids[0], series=series_v1) > + create_series_reference(msgid=msgids[0], series=series_v1, > + project=project) > > # ...and three patches > for i in range(3): > msgids.append(make_msgid()) > - create_series_reference(msgid=msgids[-1], series=series_v1) > + create_series_reference(msgid=msgids[-1], series=series_v1, > + project=project) > > # now create a new series with "cover letter" > msgids.append(make_msgid()) > series_v2 = create_series(project=project) > - ref_v2 = create_series_reference(msgid=msgids[-1], series=series_v2) > + ref_v2 = create_series_reference(msgid=msgids[-1], series=series_v2, > + project=project) > > # ...and the "first patch" of this new series > msgid = make_msgid() > diff --git patchwork/tests/utils.py patchwork/tests/utils.py > index 577183d0..4ead1781 100644 > --- patchwork/tests/utils.py > +++ patchwork/tests/utils.py > @@ -282,8 +282,12 @@ def create_series(**kwargs): > > def create_series_reference(**kwargs): > """Create 'SeriesReference' object.""" > + project = kwargs.pop('project', create_project()) > + series = kwargs.pop('series', create_series(project=project)) > + > values = { > - 'series': create_series() if 'series' not in kwargs else None, > + 'series': series, > + 'project': project, > 'msgid': make_msgid(), > } > values.update(**kwargs) > -- > 2.21.0 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork