Page MenuHomeSoftware Heritage
Differential D2662

Fix inconsistent behavior of skipped_content_missing across backends.
ClosedPublic
Actions

Authored by vlorentz on Feb 12 2020, 4:09 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDSTO51b20164f6eb: Fix inconsistent behavior of skipped_content_missing across backends.
Summary

Two fixes:

  • in-mem ignored None keys
  • cassandra yielded input dicts as-is instead of a dict with just the hashes

Diff Detail

Repository
rDSTO Storage manager
Branch
consistent-skipped_content_missing
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10576
Build 15782: tox-on-jenkinsJenkins
Build 15781: arc lint + arc unit

Event Timeline

vlorentz created this revision.Feb 12 2020, 4:09 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 12 2020, 4:09 PM
vlorentz added a child revision: D2663: Fix FilteringProxy to not drop skipped-contents with a missing sha1_git..Feb 12 2020, 4:18 PM
swh-public-ci added a comment.Feb 12 2020, 4:19 PM

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

Harbormaster completed remote builds in B10576: Diff 9515.Feb 12 2020, 4:19 PM
olasd added a subscriber: olasd.Feb 14 2020, 4:32 PM

Looks fine save from a few comments/questions.

The "content key algortithm" stuff should probably be moved to a helper in swh.model, as we keep repeating it everywhere.

swh/storage/in_memory.py
263–266

Took me a while to understand this logic. Why not use _content_key_algorithm() here?

You could also make a dict with these keys, which would alleviate the n² nature of the lookup (even though it's probably not a big deal considering how tiny n is)

279–281

As you only iterate once, no need for the list(content)

swh/storage/tests/test_storage.py
342–355

??

olasd requested changes to this revision.Feb 14 2020, 4:32 PM
This revision now requires changes to proceed.Feb 14 2020, 4:32 PM
vlorentz updated this revision to Diff 9551.Feb 14 2020, 5:49 PM

apply comments

swh-public-ci added a comment.Feb 14 2020, 6:01 PM

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

Harbormaster completed remote builds in B10612: Diff 9551.Feb 14 2020, 6:01 PM
olasd accepted this revision.Feb 17 2020, 4:19 PM
This revision is now accepted and ready to land.Feb 17 2020, 4:19 PM
Closed by commit rDSTO51b20164f6eb: Fix inconsistent behavior of skipped_content_missing across backends. (authored by vlorentz). · Explain WhyFeb 18 2020, 1:44 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDSTO51b20164f6eb: Fix inconsistent behavior of skipped_content_missing across backends..
vlorentz removed a child revision: D2663: Fix FilteringProxy to not drop skipped-contents with a missing sha1_git..Feb 18 2020, 2:04 PM

Revision Contents
Changeset List

PathSize
swh/
storage/
cassandra/
3 lines
50 lines
tests/
54 lines

Diff 9515

View Options

swh/storage/cassandra/storage.py

# Copyright (C) 2015-2021 The Software Heritage developers # Copyright (C) 2015-2021 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version # License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import datetime import datetime
import hashlib import hashlib
from swh.model import hashutil from swh.model import hashutil
from swh.model.model import ( from swh.model.model import (
ObjectType, ObjectType,
Person, Person,
Release, Release,
Revision, Revision,
RevisionType, RevisionType,
Timestamp,
TimestampWithTimezone, TimestampWithTimezone,
) )
from swh.web.common import converters from swh.web.common import converters
def test_fmap(): def test_fmap():
assert [2, 3, None, 4] == converters.fmap(lambda x: x + 1, [1, 2, None, 3]) assert [2, 3, None, 4] == converters.fmap(lambda x: x + 1, [1, 2, None, 3])
assert [11, 12, 13] == list( assert [11, 12, 13] == list(
▲ Show 20 Lines • Show All 181 Lines • ▼ Show 20 Lines for snap in [snap_hash, None]:
actual_visit = converters.from_origin_visit(visit) actual_visit = converters.from_origin_visit(visit)
assert actual_visit == expected_visit assert actual_visit == expected_visit
def test_from_release(): def test_from_release():
"""Convert release model object to a dict should be ok""" """Convert release model object to a dict should be ok"""
ts = int(
datetime.datetime(
2015, 1, 1, 22, 0, 0, tzinfo=datetime.timezone.utc
).timestamp()
)
release_input = Release( release_input = Release(
id=hashutil.hash_to_bytes("aad23fa492a0c5fed0708a6703be875448c86884"), id=hashutil.hash_to_bytes("aad23fa492a0c5fed0708a6703be875448c86884"),
target=hashutil.hash_to_bytes("5e46d564378afc44b31bb89f99d5675195fbdf67"), target=hashutil.hash_to_bytes("5e46d564378afc44b31bb89f99d5675195fbdf67"),
target_type=ObjectType.REVISION, target_type=ObjectType.REVISION,
date=TimestampWithTimezone( date=TimestampWithTimezone.from_datetime(
timestamp=Timestamp(seconds=ts, microseconds=0), datetime.datetime(2015, 1, 1, 22, 0, 0, tzinfo=datetime.timezone.utc)
offset=0,
negative_utc=False,
), ),
ardumontUnsubmitted
Done
Inline Actions

might as well inline this, date seems only used once.

ardumont: might as well inline this, date seems only used once.
author=Person( author=Person(
name=b"author name", name=b"author name",
fullname=b"Author Name author@email", fullname=b"Author Name author@email",
email=b"author@email", email=b"author@email",
), ),
name=b"v0.0.1", name=b"v0.0.1",
message=b"some comment on release", message=b"some comment on release",
synthetic=True, synthetic=True,
Show All 16 Linesdef test_from_release():
} }
actual_release = converters.from_release(release_input) actual_release = converters.from_release(release_input)
assert actual_release == expected_release assert actual_release == expected_release
def test_from_revision_model_object(): def test_from_revision_model_object():
ts = int( date = TimestampWithTimezone.from_datetime(
datetime.datetime( datetime.datetime(2000, 1, 17, 11, 23, 54, tzinfo=datetime.timezone.utc)
2000, 1, 17, 11, 23, 54, tzinfo=datetime.timezone.utc
).timestamp()
) )
revision_input = Revision( revision_input = Revision(
directory=hashutil.hash_to_bytes("7834ef7e7c357ce2af928115c6c6a42b7e2a44e6"), directory=hashutil.hash_to_bytes("7834ef7e7c357ce2af928115c6c6a42b7e2a44e6"),
author=Person( author=Person(
name=b"Software Heritage", name=b"Software Heritage",
fullname=b"robot robot@softwareheritage.org", fullname=b"robot robot@softwareheritage.org",
email=b"robot@softwareheritage.org", email=b"robot@softwareheritage.org",
), ),
committer=Person( committer=Person(
name=b"Software Heritage", name=b"Software Heritage",
fullname=b"robot robot@softwareheritage.org", fullname=b"robot robot@softwareheritage.org",
email=b"robot@softwareheritage.org", email=b"robot@softwareheritage.org",
), ),
message=b"synthetic revision message", message=b"synthetic revision message",
date=TimestampWithTimezone( date=date,
timestamp=Timestamp(seconds=ts, microseconds=0), committer_date=date,
offset=0,
negative_utc=False,
),
committer_date=TimestampWithTimezone(
timestamp=Timestamp(seconds=ts, microseconds=0),
offset=0,
negative_utc=False,
),
synthetic=True, synthetic=True,
type=RevisionType.TAR, type=RevisionType.TAR,
parents=tuple( parents=tuple(
[ [
hashutil.hash_to_bytes("29d8be353ed3480476f032475e7c244eff7371d5"), hashutil.hash_to_bytes("29d8be353ed3480476f032475e7c244eff7371d5"),
hashutil.hash_to_bytes("30d8be353ed3480476f032475e7c244eff7371d5"), hashutil.hash_to_bytes("30d8be353ed3480476f032475e7c244eff7371d5"),
] ]
), ),
▲ Show 20 Lines • Show All 463 Lines • Show Last 20 Lines
View Options

swh/storage/in_memory.py

# Copyright (C) 2015-2021 The Software Heritage developers # Copyright (C) 2015-2021 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version # License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import datetime import datetime
import hashlib import hashlib
from swh.model import hashutil from swh.model import hashutil
from swh.model.model import ( from swh.model.model import (
ObjectType, ObjectType,
Person, Person,
Release, Release,
Revision, Revision,
RevisionType, RevisionType,
Timestamp,
TimestampWithTimezone, TimestampWithTimezone,
) )
from swh.web.common import converters from swh.web.common import converters
def test_fmap(): def test_fmap():
assert [2, 3, None, 4] == converters.fmap(lambda x: x + 1, [1, 2, None, 3]) assert [2, 3, None, 4] == converters.fmap(lambda x: x + 1, [1, 2, None, 3])
assert [11, 12, 13] == list( assert [11, 12, 13] == list(
▲ Show 20 Lines • Show All 181 Lines • ▼ Show 20 Lines for snap in [snap_hash, None]:
actual_visit = converters.from_origin_visit(visit) actual_visit = converters.from_origin_visit(visit)
assert actual_visit == expected_visit assert actual_visit == expected_visit
def test_from_release(): def test_from_release():
"""Convert release model object to a dict should be ok""" """Convert release model object to a dict should be ok"""
ts = int(
datetime.datetime(
2015, 1, 1, 22, 0, 0, tzinfo=datetime.timezone.utc
).timestamp()
)
release_input = Release( release_input = Release(
id=hashutil.hash_to_bytes("aad23fa492a0c5fed0708a6703be875448c86884"), id=hashutil.hash_to_bytes("aad23fa492a0c5fed0708a6703be875448c86884"),
target=hashutil.hash_to_bytes("5e46d564378afc44b31bb89f99d5675195fbdf67"), target=hashutil.hash_to_bytes("5e46d564378afc44b31bb89f99d5675195fbdf67"),
target_type=ObjectType.REVISION, target_type=ObjectType.REVISION,
date=TimestampWithTimezone( date=TimestampWithTimezone.from_datetime(
timestamp=Timestamp(seconds=ts, microseconds=0), datetime.datetime(2015, 1, 1, 22, 0, 0, tzinfo=datetime.timezone.utc)
offset=0,
negative_utc=False,
), ),
ardumontUnsubmitted
Done
Inline Actions

might as well inline this, date seems only used once.

ardumont: might as well inline this, date seems only used once.
author=Person( author=Person(
name=b"author name", name=b"author name",
fullname=b"Author Name author@email", fullname=b"Author Name author@email",
email=b"author@email", email=b"author@email",
), ),
name=b"v0.0.1", name=b"v0.0.1",
message=b"some comment on release", message=b"some comment on release",
synthetic=True, synthetic=True,
Show All 16 Linesdef test_from_release():
} }
actual_release = converters.from_release(release_input) actual_release = converters.from_release(release_input)
assert actual_release == expected_release assert actual_release == expected_release
def test_from_revision_model_object(): def test_from_revision_model_object():
ts = int( date = TimestampWithTimezone.from_datetime(
datetime.datetime( datetime.datetime(2000, 1, 17, 11, 23, 54, tzinfo=datetime.timezone.utc)
2000, 1, 17, 11, 23, 54, tzinfo=datetime.timezone.utc
).timestamp()
) )
revision_input = Revision( revision_input = Revision(
directory=hashutil.hash_to_bytes("7834ef7e7c357ce2af928115c6c6a42b7e2a44e6"), directory=hashutil.hash_to_bytes("7834ef7e7c357ce2af928115c6c6a42b7e2a44e6"),
author=Person( author=Person(
name=b"Software Heritage", name=b"Software Heritage",
fullname=b"robot robot@softwareheritage.org", fullname=b"robot robot@softwareheritage.org",
email=b"robot@softwareheritage.org", email=b"robot@softwareheritage.org",
), ),
committer=Person( committer=Person(
name=b"Software Heritage", name=b"Software Heritage",
fullname=b"robot robot@softwareheritage.org", fullname=b"robot robot@softwareheritage.org",
email=b"robot@softwareheritage.org", email=b"robot@softwareheritage.org",
), ),
message=b"synthetic revision message", message=b"synthetic revision message",
date=TimestampWithTimezone( date=date,
timestamp=Timestamp(seconds=ts, microseconds=0), committer_date=date,
offset=0,
negative_utc=False,
),
committer_date=TimestampWithTimezone(
timestamp=Timestamp(seconds=ts, microseconds=0),
offset=0,
negative_utc=False,
),
synthetic=True, synthetic=True,
type=RevisionType.TAR, type=RevisionType.TAR,
parents=tuple( parents=tuple(
[ [
hashutil.hash_to_bytes("29d8be353ed3480476f032475e7c244eff7371d5"), hashutil.hash_to_bytes("29d8be353ed3480476f032475e7c244eff7371d5"),
hashutil.hash_to_bytes("30d8be353ed3480476f032475e7c244eff7371d5"), hashutil.hash_to_bytes("30d8be353ed3480476f032475e7c244eff7371d5"),
] ]
), ),
▲ Show 20 Lines • Show All 463 Lines • Show Last 20 Lines
View Options

swh/storage/tests/test_storage.py

# Copyright (C) 2015-2021 The Software Heritage developers # Copyright (C) 2015-2021 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version # License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import datetime import datetime
import hashlib import hashlib
from swh.model import hashutil from swh.model import hashutil
from swh.model.model import ( from swh.model.model import (
ObjectType, ObjectType,
Person, Person,
Release, Release,
Revision, Revision,
RevisionType, RevisionType,
Timestamp,
TimestampWithTimezone, TimestampWithTimezone,
) )
from swh.web.common import converters from swh.web.common import converters
def test_fmap(): def test_fmap():
assert [2, 3, None, 4] == converters.fmap(lambda x: x + 1, [1, 2, None, 3]) assert [2, 3, None, 4] == converters.fmap(lambda x: x + 1, [1, 2, None, 3])
assert [11, 12, 13] == list( assert [11, 12, 13] == list(
▲ Show 20 Lines • Show All 181 Lines • ▼ Show 20 Lines for snap in [snap_hash, None]:
actual_visit = converters.from_origin_visit(visit) actual_visit = converters.from_origin_visit(visit)
assert actual_visit == expected_visit assert actual_visit == expected_visit
def test_from_release(): def test_from_release():
"""Convert release model object to a dict should be ok""" """Convert release model object to a dict should be ok"""
ts = int(
datetime.datetime(
2015, 1, 1, 22, 0, 0, tzinfo=datetime.timezone.utc
).timestamp()
)
release_input = Release( release_input = Release(
id=hashutil.hash_to_bytes("aad23fa492a0c5fed0708a6703be875448c86884"), id=hashutil.hash_to_bytes("aad23fa492a0c5fed0708a6703be875448c86884"),
target=hashutil.hash_to_bytes("5e46d564378afc44b31bb89f99d5675195fbdf67"), target=hashutil.hash_to_bytes("5e46d564378afc44b31bb89f99d5675195fbdf67"),
target_type=ObjectType.REVISION, target_type=ObjectType.REVISION,
date=TimestampWithTimezone( date=TimestampWithTimezone.from_datetime(
timestamp=Timestamp(seconds=ts, microseconds=0), datetime.datetime(2015, 1, 1, 22, 0, 0, tzinfo=datetime.timezone.utc)
offset=0,
negative_utc=False,
), ),
ardumontUnsubmitted
Done
Inline Actions

might as well inline this, date seems only used once.

ardumont: might as well inline this, date seems only used once.
author=Person( author=Person(
name=b"author name", name=b"author name",
fullname=b"Author Name author@email", fullname=b"Author Name author@email",
email=b"author@email", email=b"author@email",
), ),
name=b"v0.0.1", name=b"v0.0.1",
message=b"some comment on release", message=b"some comment on release",
synthetic=True, synthetic=True,
Show All 16 Linesdef test_from_release():
} }
actual_release = converters.from_release(release_input) actual_release = converters.from_release(release_input)
assert actual_release == expected_release assert actual_release == expected_release
def test_from_revision_model_object(): def test_from_revision_model_object():
ts = int( date = TimestampWithTimezone.from_datetime(
datetime.datetime( datetime.datetime(2000, 1, 17, 11, 23, 54, tzinfo=datetime.timezone.utc)
2000, 1, 17, 11, 23, 54, tzinfo=datetime.timezone.utc
).timestamp()
) )
revision_input = Revision( revision_input = Revision(
directory=hashutil.hash_to_bytes("7834ef7e7c357ce2af928115c6c6a42b7e2a44e6"), directory=hashutil.hash_to_bytes("7834ef7e7c357ce2af928115c6c6a42b7e2a44e6"),
author=Person( author=Person(
name=b"Software Heritage", name=b"Software Heritage",
fullname=b"robot robot@softwareheritage.org", fullname=b"robot robot@softwareheritage.org",
email=b"robot@softwareheritage.org", email=b"robot@softwareheritage.org",
), ),
committer=Person( committer=Person(
name=b"Software Heritage", name=b"Software Heritage",
fullname=b"robot robot@softwareheritage.org", fullname=b"robot robot@softwareheritage.org",
email=b"robot@softwareheritage.org", email=b"robot@softwareheritage.org",
), ),
message=b"synthetic revision message", message=b"synthetic revision message",
date=TimestampWithTimezone( date=date,
timestamp=Timestamp(seconds=ts, microseconds=0), committer_date=date,
offset=0,
negative_utc=False,
),
committer_date=TimestampWithTimezone(
timestamp=Timestamp(seconds=ts, microseconds=0),
offset=0,
negative_utc=False,
),
synthetic=True, synthetic=True,
type=RevisionType.TAR, type=RevisionType.TAR,
parents=tuple( parents=tuple(
[ [
hashutil.hash_to_bytes("29d8be353ed3480476f032475e7c244eff7371d5"), hashutil.hash_to_bytes("29d8be353ed3480476f032475e7c244eff7371d5"),
hashutil.hash_to_bytes("30d8be353ed3480476f032475e7c244eff7371d5"), hashutil.hash_to_bytes("30d8be353ed3480476f032475e7c244eff7371d5"),
] ]
), ),
▲ Show 20 Lines • Show All 463 Lines • Show Last 20 Lines
About · Developer information · Legal