Page MenuHomeSoftware Heritage
Differential D5576

Remove 'journal_type' argument from the CLI
ClosedPublic
Actions

Authored by vlorentz on Apr 22 2021, 4:30 PM.

Details

Reviewers
vsellier
Group Reviewers
Reviewers
Commits
rDCNT85cac362ca67: Remove 'journal_type' argument from the CLI
Summary

Motivation:

  • This simplifies the CLI, and stops leaking implementation details (what field the implementation reads, what keys contains, that keys exist at all, etc.) through this interface.
  • It also removes the need to have two clients, both reading revisions and releases.

This preserves the optimization of not deserializing unneeded values,
by passing raw message values to the worker functions; which can
deserialize it themselves if needed.

Additionally, this commit fixes some mistakes in type annotations.

Test Plan

Two tests hang. I don't understand why

Diff Detail

Repository
rDCNT Archive counters
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Apr 22 2021, 4:30 PM
vlorentz mentioned this in D5572: Implement the jounal client counting an internal property of an object.Apr 22 2021, 4:32 PM
vlorentz updated this revision to Diff 19929.Apr 22 2021, 4:34 PM

remove print

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 22 2021, 4:36 PM
Harbormaster failed remote builds in B20934: Diff 19929!
vlorentz requested review of this revision.Apr 22 2021, 4:37 PM
vlorentz updated this revision to Diff 19930.Apr 22 2021, 4:46 PM

found the bug, it was variable shadowing in handle_messages.

swh-public-ci added a comment.Apr 22 2021, 4:51 PM

Build was aborted

Patch application report for D5576 (id=19928)

Rebasing onto 5cae9b7cfe...

Current branch diff-target is up to date.
Changes applied before test
commit a4115a6091373d564a5ec4ef7333bb5126a4ed91
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Apr 22 16:28:48 2021 +0200

    Remove 'journal_type' argument from the CLI
    
    Motivation:
    
    * This simplifies the CLI, and stops leaking implementation details
      (what field the implementation reads, what keys contains, that
      keys exist at all, etc.) through this interface.
    
    * It also removes the need to have two clients, both reading revisions
      and releases.
    
    This preserves the optimization of not deserializing unneeded values,
    by passing raw message values to the worker functions; which can
    deserialize it themselves if needed.
    
    Additionally, this commit fixes some mistakes in type annotations.

Link to build: https://jenkins.softwareheritage.org/job/DCNT/job/tests-on-diff/45/
See console output for more information: https://jenkins.softwareheritage.org/job/DCNT/job/tests-on-diff/45/console

Harbormaster failed remote builds in B20933: Diff 19928!Apr 22 2021, 4:51 PM
swh-public-ci added a comment.Apr 22 2021, 5:11 PM

Build was aborted

Patch application report for D5576 (id=19929)

Rebasing onto 5cae9b7cfe...

Current branch diff-target is up to date.
Changes applied before test
commit 9aba347bbdf7914fd0c3924770ddd6fa01f91d74
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Apr 22 16:28:48 2021 +0200

    Remove 'journal_type' argument from the CLI
    
    Motivation:
    
    * This simplifies the CLI, and stops leaking implementation details
      (what field the implementation reads, what keys contains, that
      keys exist at all, etc.) through this interface.
    
    * It also removes the need to have two clients, both reading revisions
      and releases.
    
    This preserves the optimization of not deserializing unneeded values,
    by passing raw message values to the worker functions; which can
    deserialize it themselves if needed.
    
    Additionally, this commit fixes some mistakes in type annotations.

Link to build: https://jenkins.softwareheritage.org/job/DCNT/job/tests-on-diff/46/
See console output for more information: https://jenkins.softwareheritage.org/job/DCNT/job/tests-on-diff/46/console

swh-public-ci added a comment.Apr 22 2021, 5:12 PM

Build is green

Patch application report for D5576 (id=19930)

Rebasing onto 5cae9b7cfe...

Current branch diff-target is up to date.
Changes applied before test
commit e1c8e4d8533e978e067bbc891b345f742c01e874
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Apr 22 16:28:48 2021 +0200

    Remove 'journal_type' argument from the CLI
    
    Motivation:
    
    * This simplifies the CLI, and stops leaking implementation details
      (what field the implementation reads, what keys contains, that
      keys exist at all, etc.) through this interface.
    
    * It also removes the need to have two clients, both reading revisions
      and releases.
    
    This preserves the optimization of not deserializing unneeded values,
    by passing raw message values to the worker functions; which can
    deserialize it themselves if needed.
    
    Additionally, this commit fixes some mistakes in type annotations.

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

Harbormaster completed remote builds in B20935: Diff 19930.Apr 22 2021, 5:12 PM
vsellier accepted this revision.Apr 22 2021, 6:46 PM

I suppose this implementation will be less effective in term of memory consumption as we will keep a copy of the message contents on the dict (I suppose the message.value() is returning a copy of the content)
It also makes the module aware of how the objects are serialized on kafka, which looks quite low level.

It's more soft considerations than real concerns no objections to have only one entry point (there is also one other pros as it will avoid to update the puppet configuration to activate the person's couter)

This revision is now accepted and ready to land.Apr 22 2021, 6:46 PM
vlorentz added a comment.Apr 22 2021, 8:18 PM

It also makes the module aware of how the objects are serialized on kafka, which looks quite low level.

Yeah :/

But swh.counters is already breaking the journal abstraction by accessing keys, so I figured I might as well do this too

vlorentz added a comment.Apr 22 2021, 8:20 PM
In D5576#141650, @vsellier wrote:

I suppose the message.value() is returning a copy of the content

It does not: https://github.com/confluentinc/confluent-kafka-python/blob/b7f8dce998ec254f54c15e514850d7404c6a71a3/src/confluent_kafka/src/confluent_kafka.c#L439-L445

(Py_INCREF only increments the reference counter)

vlorentz updated this revision to Diff 19940.Apr 22 2021, 8:21 PM

typo

This revision was landed with ongoing or failed builds.Apr 22 2021, 8:21 PM
Closed by commit rDCNT85cac362ca67: Remove 'journal_type' argument from the CLI (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDCNT85cac362ca67: Remove 'journal_type' argument from the CLI.
swh-public-ci added a comment.Apr 22 2021, 8:22 PM

Build is green

Patch application report for D5576 (id=19940)

Rebasing onto 85cac362ca...

First, rewinding head to replay your work on top of it...
Fast-forwarded diff-target to base-revision-48-D5576.
Changes applied before test



See https://jenkins.softwareheritage.org/job/DCNT/job/tests-on-diff/48/ for more details.
Harbormaster completed remote builds in B20945: Diff 19940.Apr 22 2021, 8:22 PM
vsellier added a comment.Apr 23 2021, 9:55 AM


In D5576#141670, @vlorentz wrote:




In D5576#141650, @vsellier wrote:


I suppose the message.value() is returning a copy of the content





It does not: https://github.com/confluentinc/confluent-kafka-python/blob/b7f8dce998ec254f54c15e514850d7404c6a71a3/src/confluent_kafka/src/confluent_kafka.c#L439-L445



(Py_INCREF only increments the reference counter)





thanks for the pointer
vsellier mentioned this in T3251: Count authors from revisions and releases.Apr 23 2021, 11:16 AM
Revision Contents
Changeset List
PathSize
3 lines
swh/
counters/
32 lines
27 lines
18 lines
tests/
56 lines
80 lines
Diff 19941
View Options
mypy.ini
Loading...
View Options
swh/counters/cli.py
Loading...
View Options
swh/counters/journal_client.py
Loading...
View Options
swh/counters/kafka_client.py
Loading...
View Options
swh/counters/tests/test_cli.py
Loading...
View Options
swh/counters/tests/test_journal_client.py
Loading...
About · Developer information · Legal