Page MenuHomeSoftware Heritage
Differential D859

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

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

Details

Reviewers
olasd
douardda
vlorentz
Group Reviewers
Reviewers
Commits
rDSTOCfc7c534430e8: listener: Make kafka client be less verbose
rDSTOCa229a07e1f8b: listener: Add logging behavior
rDSTOC2a03e1c5e10d: listener: Adapt decoding behavior depending on the object type
rDSTOCd12471087e91: listener: Use swh.model.hashutil.hash_to_bytes
R65:2a03e1c5e10d: listener: Adapt decoding behavior depending on the object type
R65:fc7c534430e8: listener: Make kafka client be less verbose
R65:d12471087e91: listener: Use swh.model.hashutil.hash_to_bytes
R65:a229a07e1f8b: listener: Add logging behavior
rDSTOfc7c534430e8: listener: Make kafka client be less verbose
rDSTOa229a07e1f8b: listener: Add logging behavior
rDSTOd12471087e91: listener: Use swh.model.hashutil.hash_to_bytes
rDSTO2a03e1c5e10d: listener: Adapt decoding behavior depending on the object type
Summary

Was broken since the origin/origin-visit subscription.

This fixes it.

Test Plan

tox

Diff Detail

Repository
rDSTO Storage manager
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3193
Build 4094: tox-on-jenkinsJenkins
Build 4093: arc lint + arc unit

Event Timeline

ardumont created this revision.Dec 19 2018, 8:39 PM
swh-public-ci added a subscriber: swh-public-ci.Dec 19 2018, 8:41 PM

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/140/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/140/console

Harbormaster failed remote builds in B3184: Diff 2741!Dec 19 2018, 8:41 PM
ardumont added a child revision: D860: docker-compose: Add journal stack.Dec 19 2018, 8:43 PM
ardumont updated this revision to Diff 2743.Dec 19 2018, 11:03 PM
  • tox.ini: Add missing dependency
swh-public-ci added a comment.Dec 19 2018, 11:07 PM

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

Harbormaster completed remote builds in B3186: Diff 2743.Dec 19 2018, 11:07 PM
douardda requested changes to this revision.Dec 20 2018, 10:14 AM
douardda added inline comments.
swh/storage/listener.py
27–50

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

121–137

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
27–50

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

121–137

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

ardumont added a comment.Dec 20 2018, 10:59 AM

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
swh-public-ci added a comment.Dec 20 2018, 11:03 AM

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

Harbormaster completed remote builds in B3190: Diff 2746.Dec 20 2018, 11:03 AM
ardumont marked an inline comment as done.Dec 20 2018, 11:09 AM
ardumont added inline comments.
swh/storage/listener.py
41

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
41

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.

67

"Registering to" or "Registered to"

73–75

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
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
41

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
73–75

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
swh-public-ci added a comment.Dec 20 2018, 11:55 AM

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

Harbormaster completed remote builds in B3192: Diff 2748.Dec 20 2018, 11:56 AM
vlorentz accepted this revision.Dec 20 2018, 12:12 PM
vlorentz added inline comments.
swh/storage/listener.py
47

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
47

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
swh-public-ci added a comment.Dec 20 2018, 1:14 PM

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

Harbormaster completed remote builds in B3193: Diff 2749.Dec 20 2018, 1:14 PM
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
Closed by commit rDSTO2a03e1c5e10d: listener: Adapt decoding behavior depending on the object type (authored by ardumont). · Explain WhyDec 20 2018, 2:48 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
storage/
63 lines
tests/
46 lines
2 lines

Diff 2749

View Options

swh/storage/listener.py

Loading...
View Options

swh/storage/tests/test_listener.py

Loading...
View Options

tox.ini

Loading...
About · Developer information · Legal