Page MenuHomeSoftware Heritage
Differential D7907

cli.db: Use attribute current_version instead of undeclared getter
ClosedPublic
Actions

Authored by ardumont on May 30 2022, 4:43 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T4284: scrubber does not comply to what's expected by the swh db tooling
Commits
rDCORE5cda0ca62601: cli.db: Use attribute current_version instead of undeclared getter
Summary

I have no idea why both exist. But this centralized one (current_version) is declared on
most swh modules vs the other method which is not [2]

[1]

$SWH_ENVIRONMENT_HOME/swh-indexer/swh/indexer/storage/db.py:    current_version = 133
$SWH_ENVIRONMENT_HOME/swh-scheduler/swh/scheduler/backend.py:    current_version = 33
$SWH_ENVIRONMENT_HOME/swh-scrubber/swh/scrubber/db.py:    current_version = 2
$SWH_ENVIRONMENT_HOME/swh-storage/swh/storage/postgresql/db.py:    current_version = 182
$SWH_ENVIRONMENT_HOME/swh-vault/swh/vault/backend.py:    current_version = 4

[2]

grep -r "get_current_version(" $SWH_ENVIRONMENT_HOME/*/swh/
$SWH_ENVIRONMENT_HOME/swh-indexer/swh/indexer/storage/__init__.py:    def get_current_version(self, *, db=None, cur=None):
$SWH_ENVIRONMENT_HOME/swh-scheduler/swh/scheduler/backend.py:    def get_current_version(self):
$SWH_ENVIRONMENT_HOME/swh-storage/swh/storage/postgresql/storage.py:    def get_current_version(self, *, db: Db, cur=None):

Related to T4284
Supersedes D7906

Test Plan

Patched on scrubber0.staging and now the cli works! [1]

[1]

swhworker@scrubber0:~$ swh db --config-file config.yml version scrubber
module: scrubber
current code version: 2
version: 1

Diff Detail

Unit TestsFailed
View All

TimeTest
884 msJenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_create_and_init_db_new_api
cli_runner = <click.testing.CliRunner object at 0x7f9b0dfb1630> postgresql = <connection object at 0x7f9b10c0f408; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=18610 options=''', closed: 0> mock_import_swhmodule = <function mock_import_swhmodule.<locals>.import_swhmodule_mock at 0x7f9b0dfeb6a8>
739 msJenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.db.tests.test_cli::test_cli_swh_db_upgrade_new_api
cli_runner = <click.testing.CliRunner object at 0x7f9b0dfc9d68> postgresql = <connection object at 0x7f9b1163d188; dsn: 'user=postgres password=xxx dbname=tests host=127.0.0.1 port=18610 options=''', closed: 0> datadir = '/var/lib/jenkins/workspace/DCORE/tests-on-diff/.tox/py3-core-db-server-github-slow-cover/lib/python3.7/site-packages/swh/core/db/tests/data'
1 msJenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.api.tests.test_async::test_async_decode_data_empty_cases[None]
1 msJenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.api.tests.test_async::test_async_decode_data_empty_cases[]
1 msJenkins > .tox.py3-core-db-server-github-slow-cover.lib.python3.7.site-packages.swh.core.api.tests.test_async::test_async_decode_data_empty_cases[data2]
View Full Test Results (2 Failed · 296 Passed · 1 Skipped)

Event Timeline

ardumont created this revision.May 30 2022, 4:43 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMay 30 2022, 4:43 PM
swh-public-ci added a comment.May 30 2022, 4:45 PM

Build has FAILED

Patch application report for D7907 (id=28511)

Rebasing onto f18016af5c...

Current branch diff-target is up to date.
Changes applied before test
commit c52516feac901aa72cce833bdd9d06ebb01bfd52
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 30 16:14:06 2022 +0200

    cli.db: Use attribute current_version instead of undeclared getter
    
    Related to T4284

Link to build: https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/430/
See console output for more information: https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/430/console

Harbormaster returned this revision to the author for changes because remote builds failed.May 30 2022, 4:45 PM
Harbormaster failed remote builds in B29601: Diff 28511!
ardumont updated this revision to Diff 28513.May 30 2022, 4:56 PM

Fix tests

swh-public-ci added a comment.May 30 2022, 4:58 PM

Build is green

Patch application report for D7907 (id=28513)

Rebasing onto f18016af5c...

Current branch diff-target is up to date.
Changes applied before test
commit 5cda0ca626010cccc7e7f3f73fbfb1b20702ac40
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Mon May 30 16:14:06 2022 +0200

    cli.db: Use attribute current_version instead of undeclared getter
    
    Related to T4284

See https://jenkins.softwareheritage.org/job/DCORE/job/tests-on-diff/431/ for more details.

Harbormaster completed remote builds in B29603: Diff 28513.May 30 2022, 4:58 PM
ardumont requested review of this revision.May 30 2022, 4:58 PM
ardumont edited the summary of this revision. (Show Details)May 30 2022, 4:58 PM
ardumont edited the summary of this revision. (Show Details)May 30 2022, 5:01 PM
ardumont mentioned this in D7906: db: Provide a default get_current_version method to db classes.
vlorentz accepted this revision.May 30 2022, 5:04 PM
vlorentz added a subscriber: vlorentz.

thx

This revision is now accepted and ready to land.May 30 2022, 5:04 PM
ardumont edited the test plan for this revision. (Show Details)May 30 2022, 5:05 PM
Closed by commit rDCORE5cda0ca62601: cli.db: Use attribute current_version instead of undeclared getter (authored by ardumont). · Explain WhyMay 30 2022, 5:06 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDCORE5cda0ca62601: cli.db: Use attribute current_version instead of undeclared getter.
ardumont added a reverting change: D7943: Revert "cli.db: Use attribute current_version instead of undeclared getter".Jun 2 2022, 11:42 AM
ardumont mentioned this in D7949: db.BaseDb: Propose default get_current_version method implementation.Jun 2 2022, 5:23 PM

Revision Contents
Changeset List

PathSize
swh/
core/
cli/
14 lines

Diff 28511

View Options

swh/core/cli/db.py

  • This file was added.
# Copyright (C) 2020 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import json
from pathlib import Path
import sqlite3
from swh.model.identifiers import CONTENT, SWHID, parse_swhid
from swh.web.client.client import typify
class Cache:
def __init__(self, cache_dir):
seirlUnsubmitted
Done
Inline Actions

I would like this class to be more pythonic by working as a context manager. For me, the sqlite3 connection should probably happen in an __enter__() function, and closing it in an __exit__() function. I know you cannot use a with block in the Fuse operation class (a shame...), but at least it keeps the quirky aspect of this solely in the Fuse class.

seirl: I would like this class to be more pythonic by working as a context manager. For me, the…
seirlUnsubmitted
Done
Inline Actions

As discussed, we probably want to have two caches (they can probably both inherit some common behavior from an abstract base if that's helpful).

If you do go ahead with two classes, you will be able to use getitem and setitem to enable a more pythonic way of accessing this cache, like blob_cache[swhid] = X.

seirl: As discussed, we probably want to have two caches (they can probably both inherit some common…
if cache_dir == ":memory:":
self.conn = sqlite3.connect(":memory:")
else:
cache_path = Path(cache_dir, "cache.sqlite")
cache_path.parents[0].mkdir(parents=True, exist_ok=True)
self.conn = sqlite3.connect(cache_path)
self.db = self.conn.cursor()
self.db.execute("create table if not exists metadata_cache (swhid, metadata)")
self.db.execute("create table if not exists blob_cache (swhid, blob)")
self.conn.commit()
zackUnsubmitted
Done
Inline Actions

if the sqlite3 module is a well-behaved Python DB module, this could be better written with a contextmanager that commits at the end

zack: if the sqlite3 module is a well-behaved Python DB module, this could be better written with a…
seirlUnsubmitted
Done
Inline Actions

I think it's weird to keep a long-lived cursor in the class, you should probably create a new one whenever you're executing something without the conn.execute*( shortcuts. I think the class should only hold the connection as its state, not the currently active cursors.

seirl: I think it's weird to keep a long-lived cursor in the class, you should probably create a new…
def get_metadata(self, swhid: SWHID):
zackUnsubmitted
Done
Inline Actions

Please add docstring to all public methods, classes and modules. No need to be very detailed for now, oneliners would do for starters. But it's much better to have them since the beginning than having to add all of them later.

zack: Please add docstring to all public methods, classes and modules. No need to be very detailed…
self.db.execute(
"select metadata from metadata_cache where swhid=?", (str(swhid),)
)
cache = self.db.fetchone()
if cache:
metadata = json.loads(cache[0])
return typify(metadata, swhid.object_type)
def put_metadata(self, swhid: SWHID, metadata):
zackUnsubmitted
Done
Inline Actions

methods that do not return anything should be explicitly annotated with -> None, otherwise the default return value is Any (h/t @olasd )

zack: methods that do not return anything should be explicitly annotated with `-> None`, otherwise…
self.db.execute(
"insert into metadata_cache values (?, ?)",
(
str(swhid),
json.dumps(
metadata,
# Converts the typified JSON to plain str version
default=lambda x: x.object_id
if isinstance(x, SWHID)
else x.__dict__,
),
),
)
self.conn.commit()
def get_metadata_swhids(self):
self.db.execute("select swhid from metadata_cache")
swhids = self.db.fetchall()
swhids = [parse_swhid(x[0]) for x in swhids]
return swhids
def get_blob(self, swhid: SWHID):
if swhid.object_type != CONTENT:
zackUnsubmitted
Done
Inline Actions

We might want to fix this in swh-web-client, making typify optional in return values, as it's really silly to typify there and goes the other way here. Can you file a dedicated task for this?

zack: We might want to fix this in swh-web-client, making typify optional in return values, as it's…
haltodeAuthorUnsubmitted
Done
Inline Actions

Filed in: T2646.

haltode: Filed in: T2646.
raise AttributeError("Cannot retrieve blob from non-content object type")
seirlUnsubmitted
Done
Inline Actions

While this fix is here temporarily, make it less hard to read by using parentheses, it's not obvious what the "if" applies to here.

seirl: While this fix is here temporarily, make it less hard to read by using parentheses, it's not…
self.db.execute("select blob from blob_cache where swhid=?", (str(swhid),))
cache = self.db.fetchone()
if cache:
blob = cache[0]
return blob
def put_blob(self, swhid: SWHID, blob):
self.db.execute("insert into blob_cache values (?, ?)", (str(swhid), blob))
self.conn.commit()
About · Developer information · Legal