Page MenuHomeSoftware Heritage
Differential D3117

(fix ci) serializers: Make kafka_to_key implem compatible with stable version
ClosedPublic
Actions

Authored by ardumont on May 4 2020, 3:17 PM.

Details

Reviewers
douardda
olasd
Group Reviewers
Reviewers
Commits
rDJNLfa9ab16c3055: serializers: Make kafka_to_key implem compatible with stable version
Summary

The current debian storage build on stable [1] fails on an unexposed parameter.
The current debian journal build passed on stable probably because it missed a
test on that particular method.

This commit removes the need to turn off such parameter (default to True). It's
not needed for the kafka objects because it prevents the use of dict with null
keys. This is not the case for our current kafka objects.

This also allows to be compatible with current version onward. In effect,
fixing the failed build.

[1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DSTO/job/gbp-buildpackage/177/console

Test Plan

tox

Diff Detail

Event Timeline

ardumont created this revision.May 4 2020, 3:17 PM
swh-public-ci added a comment.May 4 2020, 3:19 PM

Build is green

Patch application report for D3117 (id=11075)

Rebasing onto 01fd3f90b2...

Current branch diff-target is up to date.
Changes applied before test
commit 942a89e9c7f1cd32d538eaf593f5d2ca293e1a7f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 4 15:13:49 2020 +0200

    serializers: Make kafka_to_key implem compatible with stable version
    
    The current debian storage build on stable [1] fails on unexposed parameter.
    The current journal build on stable probably because it missed a test on that
    method.
    
    [1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DSTO/job/gbp-buildpackage/177/console

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

Harbormaster completed remote builds in B12244: Diff 11075.May 4 2020, 3:19 PM
ardumont added inline comments.May 4 2020, 3:20 PM
swh/journal/tests/test_serializers.py
71

Note that i tried to mock my way into testing the compatibility behavior.

But i hit a recursion problem. Since the current tox uses a msgpack version > 6
which has that optional (strict_map_key=False).

def test_kafka_to_key_compatibility(mocker):
    """Backward compatible back and forth serialization with keys

    This can be removed when msgpack > 5.6 is released (alongside try:except stanza in
    production code)

    """
    keys = [
        b"foo",
        b"bar",
        b"baz",
    ]

    def altered_msgpack_loads(key, raw, **kwargs):
        """For test purposes"""
        if 'strict_map_key' in kwargs:
            raise TypeError('Unsupported method')
        import msgpack
        return msgpack.loads(key, raw)

    m = mocker.patch('swh.journal.serializers.msgpack.loads')
    m.side_effect = altered_msgpack_loads

    for key in keys:
        ktk = serializers.key_to_kafka(key)
        v = serializers.kafka_to_key(ktk)

        assert v == key

    import pdb; pdb.set_trace()

    assert m.called
ardumont added a comment.May 4 2020, 3:21 PM

Now i realize, removing the explicit strict_map_key=False (default in current version).
Which would then be simpler...

ardumont added inline comments.May 4 2020, 3:24 PM
swh/journal/serializers.py
102

As explained below, removing strict_map_key=False (default in superior version) should be enough.

ardumont edited the summary of this revision. (Show Details)May 4 2020, 3:28 PM
vlorentz added a subscriber: vlorentz.May 4 2020, 3:36 PM
vlorentz added inline comments.
swh/journal/serializers.py
104

Should mention if it's >=5.6 or <=5.6

ardumont updated this revision to Diff 11076.May 4 2020, 3:39 PM

Update according to irc discussion

ardumont edited the summary of this revision. (Show Details)May 4 2020, 3:39 PM
ardumont edited the summary of this revision. (Show Details)

@vlorentz thanks ;)

swh-public-ci added a comment.May 4 2020, 3:40 PM

Build is green

Patch application report for D3117 (id=11076)

Rebasing onto 01fd3f90b2...

Current branch diff-target is up to date.
Changes applied before test
commit 5ece4abf441c7571d7bb7bb1fb078f234ef1683f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 4 15:13:49 2020 +0200

    serializers: Make kafka_to_key implem compatible with stable version
    
    The current debian storage build on stable [1] fails on an unexposed parameter
    yet. The current journal build on stable probably because it missed a test on
    that method.
    
    This commits removes the need to turn off such parameter. It's not needed for
    the kafka objects (no dict with null keys). Which allows to be compatible with
    current version onward as well.
    
    [1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DSTO/job/gbp-buildpackage/177/console

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

Harbormaster completed remote builds in B12245: Diff 11076.May 4 2020, 3:40 PM
ardumont updated this revision to Diff 11077.May 4 2020, 3:43 PM
ardumont edited the summary of this revision. (Show Details)

Improve commit message

ardumont edited the summary of this revision. (Show Details)May 4 2020, 3:43 PM
swh-public-ci added a comment.May 4 2020, 3:45 PM

Build is green

Patch application report for D3117 (id=11077)

Rebasing onto 01fd3f90b2...

Current branch diff-target is up to date.
Changes applied before test
commit f65f116d2cb885ea80fcc4923202e984fc3cbdf1
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 4 15:13:49 2020 +0200

    serializers: Make kafka_to_key implem compatible with stable version
    
    The current debian storage build on stable [1] fails on an unexposed parameter.
    The current debian journal build passed on stable probably because it missed a
    test on that particular method.
    
    This commit removes the need to turn off such parameter (default to True). It's
    not needed for the kafka objects because it prevents the use of dict with null
    keys. This is not the case for our current kafka objects.
    
    This also allows to be compatible with current version onward. In effect,
    fixing the failed build.
    
    [1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DSTO/job/gbp-buildpackage/177/console

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

Harbormaster completed remote builds in B12246: Diff 11077.May 4 2020, 3:45 PM
olasd accepted this revision.May 4 2020, 5:30 PM

test_kafka_to_key should really have more complicated inputs than just byte strings. if you keep it, I suggest using the same objects as the other test functions in that file.

This revision is now accepted and ready to land.May 4 2020, 5:30 PM
ardumont updated this revision to Diff 11080.May 4 2020, 5:46 PM
ardumont edited the summary of this revision. (Show Details)

Use all possible KeyType as tests input

swh-public-ci added a comment.May 4 2020, 5:48 PM

Build is green

Patch application report for D3117 (id=11080)

Rebasing onto 01fd3f90b2...

Current branch diff-target is up to date.
Changes applied before test
commit 87a93cb322c4d01ff5d9cd34cb69a1917ce99717
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 4 15:13:49 2020 +0200

    serializers: Make kafka_to_key implem compatible with stable version
    
    The current debian storage build on stable [1] fails on an unexposed parameter.
    The current debian journal build passed on stable probably because it missed a
    test on that particular method.
    
    This commit removes the need to turn off such parameter (default to True). It's
    not needed for the kafka objects because it prevents the use of dict with null
    keys. This is not the case for our current kafka objects.
    
    This also allows to be compatible with current version onward. In effect,
    fixing the failed build.
    
    [1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DSTO/job/gbp-buildpackage/177/console

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

Harbormaster completed remote builds in B12249: Diff 11080.May 4 2020, 5:48 PM
ardumont updated this revision to Diff 11081.May 4 2020, 5:51 PM

Add even more objects to test

swh-public-ci added a comment.May 4 2020, 5:52 PM

Build is green

Patch application report for D3117 (id=11081)

Rebasing onto 01fd3f90b2...

Current branch diff-target is up to date.
Changes applied before test
commit fa9ab16c3055e692e94040427f3ecf028de5a2bf
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon May 4 15:13:49 2020 +0200

    serializers: Make kafka_to_key implem compatible with stable version
    
    The current debian storage build on stable [1] fails on an unexposed parameter.
    The current debian journal build passed on stable probably because it missed a
    test on that particular method.
    
    This commit removes the need to turn off such parameter (default to True). It's
    not needed for the kafka objects because it prevents the use of dict with null
    keys. This is not the case for our current kafka objects.
    
    This also allows to be compatible with current version onward. In effect,
    fixing the failed build.
    
    [1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DSTO/job/gbp-buildpackage/177/console

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

Harbormaster completed remote builds in B12250: Diff 11081.May 4 2020, 5:52 PM
Closed by commit rDJNLfa9ab16c3055: serializers: Make kafka_to_key implem compatible with stable version (authored by ardumont). · Explain WhyMay 4 2020, 5:53 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDJNLfa9ab16c3055: serializers: Make kafka_to_key implem compatible with stable version.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
journal/
2 lines43%
Loading...
tests/
24 lines100%
Loading...

Diff 11080

View Options

swh/journal/serializers.py

Loading...
View Options

swh/journal/tests/test_serializers.py

Loading...
About · Developer information · Legal