Page MenuHomeSoftware Heritage

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

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

Details

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

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.

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

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

swh/journal/serializers.py
102

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

vlorentz added inline comments.
swh/journal/serializers.py
104

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

Update according to irc discussion

ardumont edited the summary of this revision. (Show Details)

@vlorentz thanks ;)

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.

ardumont edited the summary of this revision. (Show Details)

Improve commit message

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.

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 edited the summary of this revision. (Show Details)

Use all possible KeyType as tests input

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.

Add even more objects to test

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.