Page MenuHomeSoftware Heritage

listener: fix decoding behavior and add logging behavior to the service
ClosedPublic

Authored by ardumont on Dec 19 2018, 8:39 PM.

Diff Detail

Repository
rDSTO Storage manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Dec 19 2018, 8:39 PM
ardumont updated this revision to Diff 2743.Dec 19 2018, 11:03 PM
  • tox.ini: Add missing dependency
douardda requested changes to this revision.Dec 20 2018, 10:14 AM
douardda added inline comments.
swh/storage/listener.py
26–48

Why keep this function? It adds IMHO useless complexity here.

120

nitpicking: this is unrelated with the "Adapt decoding behavior depending on the object type" and should be then in a dedicated git revision.

This revision now requires changes to proceed.Dec 20 2018, 10:14 AM
ardumont marked 2 inline comments as done.Dec 20 2018, 10:52 AM
ardumont added inline comments.
swh/storage/listener.py
26–48

Yes, it was initially "more" than just json.loads.
Although, with this i don't repeat that actions.

Oh, you mean systematically call decode_with_identifier on everything...
Let me check then ;)

120

Yes, i thought i did multiple commits but somehow i did not.
Will do that indeed.

In the end, with the simplification, this diff is really about fixing the listener's decoding behavior!

(and adding some logging behavior)

ardumont updated this revision to Diff 2746.Dec 20 2018, 11:00 AM
  • listener: Adapt decoding behavior depending on the object type
  • listener: Add logging behavior
  • listener: Simplify decode behavior
ardumont retitled this revision from listener: Adapt decoding behavior depending on the object type to listener: fix decoding behavior and add logging behavior to the service.Dec 20 2018, 11:01 AM
ardumont marked an inline comment as done.Dec 20 2018, 11:09 AM
ardumont added inline comments.
swh/storage/listener.py
40

but that's not possible...
otherwise, that would not be json serializable?!

Something seems off here.
Triggers provides the id as hex already... [1]

[1] https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/sql/70-swh-triggers.sql

vlorentz requested changes to this revision.Dec 20 2018, 11:14 AM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/storage/listener.py
40

I don't understand this. First, you probably meant if isinstance(v, bytes): , because value is always a dict.

But v is produced by json.loads, so it's impossible for it to be bytes.

66

"Registering to" or "Registered to"

71–73

Not very readable. Should be a single log line, and explain what this is about.

swh/storage/tests/test_listener.py
33–35

This only tests that decode is the reverse function of json.dumps.

This revision now requires changes to proceed.Dec 20 2018, 11:14 AM
ardumont marked an inline comment as done.Dec 20 2018, 11:15 AM

@vlorentz, yes there is something off with that diff now
i think i grok what i did wrong.

It's the other way around, the hex are provided by the triggers.
So the old code was actually converting the hex ids to bytes.

And this diff broke that behavior.
I'm fixing it.

swh/storage/listener.py
40

ok, i think i messed up.
It's the other way around, it takes the ids as hex and turn them into bytes.

So yes, the complexity is needed in the end!

ardumont marked an inline comment as done.Dec 20 2018, 11:18 AM
ardumont added inline comments.
swh/storage/listener.py
71–73

This is about understanding what happens and what those things look like.
I did not remember ;)

ardumont planned changes to this revision.Dec 20 2018, 11:18 AM
ardumont updated this revision to Diff 2748.Dec 20 2018, 11:52 AM
  • listener: Adapt decoding behavior depending on the object type
  • listener: Add logging behavior
vlorentz accepted this revision.Dec 20 2018, 12:12 PM
vlorentz added inline comments.
swh/storage/listener.py
46

Use hashutil.hash_from_hex instead of bytes.fromhex.

ardumont marked an inline comment as done.Dec 20 2018, 12:13 PM
ardumont added inline comments.
swh/storage/listener.py
46

Yeah, i don't know why we used that instead of our common hashutil.

ardumont updated this revision to Diff 2749.Dec 20 2018, 1:11 PM
  • listener: Use swh.model.hashutil.hash_to_bytes
  • listener: Make kafka client be less verbose
douardda accepted this revision.Dec 20 2018, 2:45 PM
This revision is now accepted and ready to land.Dec 20 2018, 2:45 PM
This revision was automatically updated to reflect the committed changes.