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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3186
Build 4083: tox-on-jenkinsJenkins
Build 4082: arc lint + arc unit

Event Timeline

  • tox.ini: Add missing dependency
douardda added inline comments.
swh/storage/listener.py
26–36

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

139–152

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 added inline comments.
swh/storage/listener.py
26–36

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 ;)

139–152

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)

  • 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 added inline comments.
swh/storage/listener.py
36

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 added a subscriber: vlorentz.
vlorentz added inline comments.
swh/storage/listener.py
36

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.

76

"Registering to" or "Registered to"

82–84

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

swh/storage/tests/test_listener.py
32–34

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

@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
36

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 added inline comments.
swh/storage/listener.py
82–84

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

  • listener: Adapt decoding behavior depending on the object type
  • listener: Add logging behavior
vlorentz added inline comments.
swh/storage/listener.py
42

Use hashutil.hash_from_hex instead of bytes.fromhex.

ardumont added inline comments.
swh/storage/listener.py
42

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

  • listener: Use swh.model.hashutil.hash_to_bytes
  • listener: Make kafka client be less verbose
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.