Page MenuHomeSoftware Heritage
Differential D986

Add a new indexer that combines all metadata-related indexers in a single task.
ClosedPublic
Actions

Authored by vlorentz on Jan 22 2019, 3:01 PM.

Details

Reviewers
douardda
ardumont
Group Reviewers
Reviewers
Commits
rDCIDXc4c70fc6bd1d: Add a new indexer that combines all metadata-related indexers in a single task.
Summary

Scheduling a task at the end of each origin-head and revision-metadata task
was way too slow (approx. 5 seconds, see T1492).

This Diff adds a new indexer that calls the three other indexers without
going through the scheduler, which is a lot faster and requires less code.

Further Diffs will clean up the code that is now useless.

Test Plan

tox + docker

Diff Detail

Repository
rDCIDX Metadata indexer
Branch
full-origin-metadata-indexer
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3655
Build 4757: tox-on-jenkinsJenkins
Build 4756: arc lint + arc unit

Event Timeline

vlorentz created this revision.Jan 22 2019, 3:01 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJan 22 2019, 3:01 PM
Harbormaster failed remote builds in B3652: Diff 3127!Jan 22 2019, 3:04 PM
swh-public-ci added a subscriber: swh-public-ci.Jan 22 2019, 3:04 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/242/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tox/242/console

swh-public-ci added a comment.Jan 22 2019, 3:13 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/243/ for more details.

vlorentz added a child revision: D987: Drop scheduling logic from the metadata indexers..Jan 22 2019, 3:22 PM
douardda requested changes to this revision.Jan 23 2019, 9:41 AM
douardda added a subscriber: douardda.

This kind of refactoring needs a bit of thought... The problem addressed here is a performance problem of the scheduler API, it seems. This response is presented as a workaround for that. I'd like to have better arguments than that to justify this refactoring.

The proper question to answer requires to have 'profiling' data: what is the 'payload'/'task management scaffolding' ratio? How long these other indexers take? Does it make sense to go though the scheduler for (very) short tasks? What 'short' means in this context?

Note that I don't mean this refactoring must not be done. I'd like to have strong arguments. And a beginning of reflexion on this scheduling problem.

swh/indexer/tests/test_origin_metadata.py
144

print?

196–198

same ? as above

This revision now requires changes to proceed.Jan 23 2019, 9:41 AM
vlorentz updated this revision to Diff 3130.Jan 23 2019, 10:35 AM
  • Remove prints.
swh-public-ci added a comment.Jan 23 2019, 10:38 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/246/ for more details.

Harbormaster completed remote builds in B3655: Diff 3130.Jan 23 2019, 10:38 AM
vlorentz added a comment.Jan 23 2019, 10:53 AM
In D986#21034, @douardda wrote:

The proper question to answer requires to have 'profiling' data: what is the 'payload'/'task management scaffolding' ratio?

Task management is two parts: running each of the three tasks in the pipeline and scheduling a new task at the end of OriginHeadIndexer and RevisionMetadataIndexer tasks. The former is fast enough, the latter takes 5 seconds twice.

The payload is approximately 1 second; so that's a 1/10 ratio.

How long these other indexers take?

What do you mean?

Does it make sense to go though the scheduler for (very) short tasks?
What 'short' means in this context?

No it doesn't. I decided early on to run these three indexers in separate tasks because it made sense "logically" (they do three separate things); but with time I no longer see the point.
There are still very connected and pass data to each other.

"Formerly", there were 4 read requests to the graph storage and 7 requests to the indexer storage. With this new indexer: 3 read requests to the graph storage and 5 to the indexer storage. Plus 1 read request to the objstorage for both.
Computation time is dominated by these either way.

There difference in the number of request is due to the different parts being able to pass data directly to each other instead of re-fecthing it, as they no longer communicate through the scheduler.

vlorentz updated this revision to Diff 3131.Jan 23 2019, 10:53 AM
  • Remove another print.
swh-public-ci added a comment.Jan 23 2019, 10:56 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/247/ for more details.

Harbormaster completed remote builds in B3656: Diff 3131.Jan 23 2019, 10:56 AM
vlorentz edited the summary of this revision. (Show Details)Jan 24 2019, 1:24 PM
douardda requested changes to this revision.Jan 24 2019, 5:28 PM
douardda added inline comments.
swh/indexer/metadata.py
335–341

why not overload the prepare() method?

343–345

kill this method and add a USE_TOOLS = False class attribute, see D990

347–349

Should not be required any more, see D990

swh/indexer/tests/test_origin_metadata.py
123–126

If you wait for D993 (like 1/2h), I killed these awful XXXTestIndexer...

This revision now requires changes to proceed.Jan 24 2019, 5:28 PM
vlorentz updated this revision to Diff 3193.Jan 25 2019, 9:57 AM
vlorentz marked 3 inline comments as done.
  • Rebase
  • Apply changes required by D990.
vlorentz marked 2 inline comments as done.Jan 25 2019, 9:57 AM
vlorentz added inline comments.
swh/indexer/metadata.py
335–341

So tests can override just this part without reimplementing the whole prepare(). Though it should eventually go away

vlorentz marked an inline comment as done.Jan 25 2019, 9:58 AM
vlorentz added inline comments.
swh/indexer/tests/test_origin_metadata.py
123–126

Not all of them; OriginHeadTestIndexer and RevisionMetadataTestIndexer are used as "sub-indexer" by this test and test_pipeline.

swh-public-ci added a comment.Jan 25 2019, 10:00 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/271/ for more details.

Harbormaster completed remote builds in B3721: Diff 3193.Jan 25 2019, 10:00 AM
vlorentz marked an inline comment as done.Jan 25 2019, 10:10 AM
vlorentz added inline comments.
swh/indexer/tests/test_origin_metadata.py
123–126

(I'm working on it)

vlorentz updated this revision to Diff 3195.Jan 25 2019, 10:19 AM
vlorentz mentioned this in D1008: Kill RevisionMetadataTestIndexer and one of the OriginHeadTestIndexers..
vlorentz added a parent revision: D1008: Kill RevisionMetadataTestIndexer and one of the OriginHeadTestIndexers..Jan 25 2019, 10:20 AM
swh-public-ci added a comment.Jan 25 2019, 10:25 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/273/ for more details.

Harbormaster completed remote builds in B3723: Diff 3195.Jan 25 2019, 10:25 AM
vlorentz added a child revision: D1010: Make metadata indexers store the mappings used to translate metadata..Jan 25 2019, 3:38 PM
ardumont accepted this revision.Jan 28 2019, 3:10 PM
vlorentz updated this revision to Diff 3221.Jan 28 2019, 4:02 PM
  • rebase
swh-public-ci added a comment.Jan 28 2019, 4:05 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/280/ for more details.

Harbormaster completed remote builds in B3758: Diff 3221.Jan 28 2019, 4:05 PM
douardda requested changes to this revision.Jan 29 2019, 9:51 AM

Minor stuffs to fix then we are good to go.

swh/indexer/metadata.py
335–341

Did I miss something? I do not see this method being overloaded in the tests below.

swh/indexer/tests/test_origin_metadata.py
13

Please try to keep your imports grouped and sorted

This revision now requires changes to proceed.Jan 29 2019, 9:51 AM
vlorentz updated this revision to Diff 3234.Jan 29 2019, 10:25 AM
  • Rebase
  • Remove _prepare_sub_indexers.
  • Fix rebase
  • Sort imports.
vlorentz marked 3 inline comments as done.Jan 29 2019, 10:25 AM
swh-public-ci added a comment.Jan 29 2019, 10:28 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/286/ for more details.

Harbormaster completed remote builds in B3779: Diff 3234.Jan 29 2019, 10:28 AM
ardumont accepted this revision.Jan 29 2019, 10:49 AM
vlorentz updated this revision to Diff 3238.Jan 29 2019, 11:49 AM
  • squash
swh-public-ci added a comment.Jan 29 2019, 11:52 AM

Build is green
See https://jenkins.softwareheritage.org/job/DCIDX/job/tox/287/ for more details.

Harbormaster completed remote builds in B3783: Diff 3238.Jan 29 2019, 11:52 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 29 2019, 12:06 PM
Closed by commit rDCIDXc4c70fc6bd1d: Add a new indexer that combines all metadata-related indexers in a single task. (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
indexer/
2 lines
59 lines
10 lines
tests/
81 lines

Diff 3130

View Options

swh/indexer/journal_client.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
View Options

swh/indexer/metadata.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
View Options

swh/indexer/tasks.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
View Options

swh/indexer/tests/test_origin_metadata.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
About · Developer information · Legal