Re: [PATCH 00/11] Add labels support
On 09/08/18 18:54, Daniel Axtens wrote: This series starts work on the latter of these by addressing yet another issues, #22 [3]. Full details of the feature are provided inline but tl;dr labels are arbitrary bits of metadata that can be used to represent some of the more orthogonal states like "RFC" or "Under Review" along with other maintainer-provided labels. Once we have support for this, we can build upon it to migrate some of the 'states' to labels and the 'state' field itself to a boolean field. This is all in the future though. So I haven't read through the patches in great detail, but I want to just query the idea that RFC is orthogonal. I understand a bunch of maintainers have a general policy of not merging RFC patches, so if something is posted as RFC they just mark it as RFC on Patchwork and then don't ever look at it again. + mpe: I think we touched on this issue of the orthogonality of the RFC classification when we were chatting about snowpatch things the other day? -- Andrew Donnellan OzLabs, ADL Canberra andrew.donnel...@au1.ibm.com IBM Australia Limited ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 2/3] Default dev settings, set host to empty (unix socket) on postgresql
On Fri, 10 Aug 2018 01:08:48 +1000 Daniel Axtens wrote: > Daniel Black writes: > > > An empty environment variable resulted in localhost, meaning > > posgresql connecting to domain sockets wasn't available. > > It took me a long time to understand how this works (e.g. that PGPORT > is still required to be set for psql to find the correct domain > socket.) But I think I'm now a bit clearer. > > > Signed-off-by: Daniel Black > > --- > > .travis.yml | 3 +++ > > patchwork/settings/dev.py | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/.travis.yml b/.travis.yml > > index 58e0b08..0c6e79d 100644 > > --- a/.travis.yml > > +++ b/.travis.yml > > @@ -22,6 +22,7 @@ env: > > - PW_TEST_DB_TYPE=mysql PW_TEST_DB_USER=root > >global: > > - PW_TEST_DB_PASS="" > > +- PW_TEST_DB_HOST="localhost" > > I tried setting this > to "" globally and it seemed to work; is there > any reason to treat postgres 10 and 11 specially? I was preserving PW_TEST_DB_HOST=localhost so the previous 9.6 test was run the same way as it did previously. > More fundamentally, is there any reason we particuarly want to do > things this way on Travis? No idea which "things" you're exactly referring to. > > Regards, > Daniel > ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 1/3] travis: test against postgresql 10 and 11
On Fri, 10 Aug 2018 01:12:28 +1000 Daniel Axtens wrote: > Hi Daniel, > > > +matrix: > > + include: > > +- addons: > > +postgresql: "10" > > +apt: > > + packages: > > + - postgresql-10 > > + - postgresql-client-10 > > + env: > > +- PGPORT=5433 > > +- PW_TEST_DB_PORT=5433 > > +- PW_TEST_DB_TYPE=postgres > > +- PW_TEST_DB_USER=travis > > +- PW_TEST_DB_PASS="" > > > The password is redundant here as it's also supplied in the global: > var list. quite right. > It's not documented in the commit message, but if I understand > correctly, the reason you need to specify PGPORT and PW_TEST_DB_PORT > is because the postgres server runs _in addition to_ the original 9.6 > server on 5432. So the new server picks 5433 and we need to tell psql > (PGPORT) The server installation I thought also picked up on the PGPORT. > and patchwork (PW_TEST_DB_PORT) about it. yes. > If you end up doing a respin of this series for patch 2, please change > these things. If we end up keeping patch 2 as is or dropping it > entirely, I'll just make these changes when I merge. thanks. > > + - if [[ $PW_TEST_DB_TYPE == mysql ]]; then mysql -e 'create > > database patchwork character set utf8;'; fi > > + - if [[ $PW_TEST_DB_TYPE == postgres ]]; then psql -c "create > > database patchwork with ENCODING = 'UTF8';" -U $PW_TEST_DB_USER; > > fi > > Thanks for cleaning this up. consider moving to utf8mb4 for MySQL at some point. The default is a 3 char version which doesn't cover all. > > script: > > + - > > > +if [[ $PW_TEST_DB_TYPE == mysql ]]; > > +then > > + mysql -e 'SELECT VERSION(), CURRENT_USER();' -u > > $PW_TEST_DB_USER patchwork; > > +else > > + psql -c "SELECT VERSION(), CURRENT_USER, current_database()" > > -U $PW_TEST_DB_USER patchwork; > > +fi > > I really like this bit, thanks! easy. ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH v2 1/3] travis: test against postgresql 10 and 11
Hi Daniel, > +matrix: > + include: > +- addons: > +postgresql: "10" > +apt: > + packages: > + - postgresql-10 > + - postgresql-client-10 > + env: > +- PGPORT=5433 > +- PW_TEST_DB_PORT=5433 > +- PW_TEST_DB_TYPE=postgres > +- PW_TEST_DB_USER=travis > +- PW_TEST_DB_PASS="" The password is redundant here as it's also supplied in the global: var list. It's not documented in the commit message, but if I understand correctly, the reason you need to specify PGPORT and PW_TEST_DB_PORT is because the postgres server runs _in addition to_ the original 9.6 server on 5432. So the new server picks 5433 and we need to tell psql (PGPORT) and patchwork (PW_TEST_DB_PORT) about it. If you end up doing a respin of this series for patch 2, please change these things. If we end up keeping patch 2 as is or dropping it entirely, I'll just make these changes when I merge. > + python: 3.6 > + sudo: true > + before_install: > +- sudo -u postgres psql -c "grant ALL on DATABASE postgres to travis > WITH GRANT OPTION;" > +- env: > +- PGPORT=5433 > +- PW_TEST_DB_PORT=5433 > +- PW_TEST_DB_TYPE=postgres > +- PW_TEST_DB_USER=travis > +- PW_TEST_DB_PASS="" > + python: 3.6 > + dist: trusty > + addons: > +postgresql: "11" > +apt: > + sources: > + - sourceline: 'deb http://apt.postgresql.org/pub/repos/apt/ > trusty-pgdg 11' > +key_url: 'https://www.postgresql.org/media/keys/ACCC4CF8.asc' > + packages: > + - postgresql-11 > + - postgresql-client-11 > + sudo: true > + before_install: > +- sudo -u postgres psql -c "grant ALL on DATABASE postgres to travis > WITH GRANT OPTION;" > + > + > before_script: > - - mysql -e 'create database patchwork character set utf8;' > - - psql -c "create database patchwork with ENCODING = 'UTF8';" -U postgres > + - if [[ $PW_TEST_DB_TYPE == mysql ]]; then mysql -e 'create database > patchwork character set utf8;'; fi > + - if [[ $PW_TEST_DB_TYPE == postgres ]]; then psql -c "create database > patchwork with ENCODING = 'UTF8';" -U $PW_TEST_DB_USER; fi Thanks for cleaning this up. > script: > + - > > +if [[ $PW_TEST_DB_TYPE == mysql ]]; > +then > + mysql -e 'SELECT VERSION(), CURRENT_USER();' -u $PW_TEST_DB_USER > patchwork; > +else > + psql -c "SELECT VERSION(), CURRENT_USER, current_database()" -U > $PW_TEST_DB_USER patchwork; > +fi I really like this bit, thanks! >- tox >- tox -e coverage > > -- > 2.17.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
Re: [PATCH v2 2/3] Default dev settings, set host to empty (unix socket) on postgresql
Daniel Black writes: > An empty environment variable resulted in localhost, meaning > posgresql connecting to domain sockets wasn't available. It took me a long time to understand how this works (e.g. that PGPORT is still required to be set for psql to find the correct domain socket.) But I think I'm now a bit clearer. > Signed-off-by: Daniel Black > --- > .travis.yml | 3 +++ > patchwork/settings/dev.py | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/.travis.yml b/.travis.yml > index 58e0b08..0c6e79d 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -22,6 +22,7 @@ env: > - PW_TEST_DB_TYPE=mysql PW_TEST_DB_USER=root >global: > - PW_TEST_DB_PASS="" > +- PW_TEST_DB_HOST="localhost" I tried setting this to "" globally and it seemed to work; is there any reason to treat postgres 10 and 11 specially? More fundamentally, is there any reason we particuarly want to do things this way on Travis? Regards, Daniel ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [RFC PATCH] 3x improvement in patch listing
On Thu, Aug 09, 2018 at 11:55:16AM +1000, Stewart Smith wrote: Out of interest, does the kernel.org instance back onto PostgreSQL or MySQL? We have one of each. The original instance patchwork.kernel.org is on MySQL (well, MariaDB), because that's the database cluster we have in our PDX DC. When we split the LKML project off to a VM in AWS (lore.kernel.org/patchwork), we set up a separate instance that's backed by the AWS PostgreSQL instance. However, that instance is for archival purposes only, and the one that's actually used for patch tracking and management is patchwork.kernel.org. I have a bunch of ideas that would quite dramatically improve performance on MySQL (properly using primary keys to force disk layout to be sane, rather than the current screw-locality method that Django enforces). On PostgreSQL, things are a bit different, but it's possible an occasional ALTER TABLE CLUSTER BY (or whatever the syntax is) could help a *lot*. If you want to recreate a huge project, you can feed 20 years of LKML archives into it. :) I'm not sure that archiving the patches would help query performance at all, but I'd have to check it out a bit. The query that Django generates is certainly "interesting". I know for a fact that it does, because I have empirical evidence to prove it. Go to lore.kernel.org/patchwork and try the patch listing with Archived=No and without it. There will be a few seconds difference. -K signature.asc Description: PGP signature ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH 00/11] Add labels support
Stephen Finucane writes: > I want to add support for global series states, as noted in #157 [1] > However, to get there we're going to need a boolean open/closed > attribute for 'Patch.state' and to get _there_ we're going to need to > clean up the various 'State' fixtures, as noted in #4 [2]. I'm very happy to get states into bundles and series. But I don't understand why it needs to be boolean to allow this? > This series starts work on the latter of these by addressing yet another > issues, #22 [3]. Full details of the feature are provided inline but > tl;dr labels are arbitrary bits of metadata that can be used to > represent some of the more orthogonal states like "RFC" or "Under > Review" along with other maintainer-provided labels. Once we have > support for this, we can build upon it to migrate some of the 'states' > to labels and the 'state' field itself to a boolean field. This is all > in the future though. So I haven't read through the patches in great detail, but I want to just query the idea that RFC is orthogonal. I understand a bunch of maintainers have a general policy of not merging RFC patches, so if something is posted as RFC they just mark it as RFC on Patchwork and then don't ever look at it again. > [1] https://github.com/getpatchwork/patchwork/issues/157 Regarding this one: > [2] https://github.com/getpatchwork/patchwork/issues/4 OzLabs does have some different tags to the default: e.g. "Needs Review / ACK". So we might need to reconsider the basis for this Regarding this one: > [3] https://github.com/getpatchwork/patchwork/issues/22 Do we have users wanting this one? I'm just really really leery making bigger sorts of changes until we pay down a _lot_ of our technical debt. Regards, Daniel > > Stephen Finucane (11): > fields: Add ColorField > models: Add patch labels > parser: Extract and save labels > parser: Remove matching label from subject prefixes > admin: Group register calls at bottom > admin: Add label views > views: Populate 'project' attribute of PatchForm > views: Add patch labels to web UI > REST: Expose Patch.labels > docs: Add information on labels > docs: Random fixes > > docs/api/index.rst| 2 + > docs/deployment/index.rst | 2 + > docs/development/api.rst | 6 +- > docs/development/index.rst| 2 + > docs/index.rst| 19 > docs/releases/index.rst | 2 + > docs/usage/delegation.rst | 3 - > docs/usage/index.rst | 2 + > docs/usage/overview.rst | 153 > +++--- > patchwork/admin.py| 61 +- > patchwork/api/patch.py| 12 +- > patchwork/fields.py | 27 - > patchwork/fixtures/default_labels.xml | 9 ++ > patchwork/forms.py| 44 +++- > patchwork/migrations/0026_add_patch_labels.py | 39 +++ > patchwork/models.py | 35 ++ > patchwork/parser.py | 22 > patchwork/templates/patchwork/patch-list.html | 8 ++ > patchwork/templates/patchwork/submission.html | 7 ++ > patchwork/templatetags/patch.py | 24 > patchwork/tests/test_parser.py| 23 > patchwork/tests/utils.py | 15 +++ > patchwork/views/__init__.py | 2 +- > patchwork/views/patch.py | 5 +- > 24 files changed, 369 insertions(+), 155 deletions(-) > create mode 100644 patchwork/fixtures/default_labels.xml > create mode 100644 patchwork/migrations/0026_add_patch_labels.py > > -- > 2.14.3 > > ___ > 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
[PATCH] Add covering index for /list/ query
In constructing the list of patches for a project, there are two
main queries that are executed:
1) get a count() of how many patches there are
2) Get the page of results being displayed
In a test dataset of ~11500 LKML patches and ~4000 others, the existing
code would take around 585ms and 858ms with a cold cache and 28ms and
198ms for a warm cache.
By adding a covering index, we get down to 4ms and 255ms for a cold
cache, and 4ms and 143ms for a warm cache!
Additionally, when there's a lot of archived or accepted patches
(I used ~11000 archived out of the 15000 total in my test set)
the query time goes from 28ms and 72ms down to 2ms and 33-40ms!
Signed-off-by: Stewart Smith
---
.../0028_add_list_covering_index.py | 19 +++
patchwork/models.py | 6 ++
2 files changed, 25 insertions(+)
create mode 100644 patchwork/migrations/0028_add_list_covering_index.py
diff --git a/patchwork/migrations/0028_add_list_covering_index.py
b/patchwork/migrations/0028_add_list_covering_index.py
new file mode 100644
index ..65ebaefbead7
--- /dev/null
+++ b/patchwork/migrations/0028_add_list_covering_index.py
@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.15 on 2018-08-09 17:24
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+dependencies = [
+('patchwork', '0027_add_comment_date_index'),
+]
+
+operations = [
+migrations.AddIndex(
+model_name='patch',
+index=models.Index(fields=['archived', 'patch_project', 'state',
'delegate'], name='patch_list_covering_idx'),
+),
+]
diff --git a/patchwork/models.py b/patchwork/models.py
index d2389cfdad29..15224ad69cfa 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -598,6 +598,12 @@ class Patch(SeriesMixin, Submission):
if django.VERSION >= (1, 10):
base_manager_name = 'objects'
+indexes = [
+# This is a covering index for the /list/ query
+
models.Index(fields=['archived','patch_project','state','delegate'],
+ name='patch_list_covering_idx'),
+]
+
class Comment(EmailMixin, models.Model):
# parent
--
2.17.1
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
