Page MenuHomeSoftware Heritage
Differential D4082

Make the type of values of JournalWriter generic, so it works with types not from swh-model.
ClosedPublic
Actions

Authored by vlorentz on Sep 29 2020, 3:58 PM.

Details

Reviewers
ardumont
douardda
Group Reviewers
Reviewers
Maniphest Tasks
T2651: Make the indexer-storage publish its rows to Kafka
Commits
rDJNL1a6c825884f3: Make the type of values of JournalWriter generic, so it works with types not…
Summary

The indexer-storage will use this.

Depends on D4080

Diff Detail

Repository
rDJNL Journal infrastructure
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.Sep 29 2020, 3:58 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 29 2020, 3:58 PM
vlorentz planned changes to this revision.Sep 29 2020, 3:58 PM
swh-public-ci added a comment.Sep 29 2020, 3:59 PM

Build has FAILED

Patch application report for D4082 (id=14401)

Could not rebase; Attempt merge onto 7815cad28a...

Updating 7815cad..aad6a07
Fast-forward
 swh/journal/pytest_plugin.py           |   4 +-
 swh/journal/serializers.py             | 109 +--------------------------------
 swh/journal/tests/journal_data.py      |   4 +-
 swh/journal/tests/test_kafka_writer.py |  17 +++--
 swh/journal/tests/test_serializers.py  |  11 +---
 swh/journal/writer/__init__.py         |  29 +++++++++
 swh/journal/writer/inmemory.py         |  20 +++---
 swh/journal/writer/kafka.py            |  52 ++++++++--------
 8 files changed, 89 insertions(+), 157 deletions(-)
Changes applied before test
commit aad6a0740f57896075137f0d758b500cc5d91f18
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:58:07 2020 +0200

    Make the type of values of JournalWriter generic, so it works with types not from swh-model.
    
    The indexer-storage will use this.

commit 549624eff1ee246f0f8552491f730123520376e8
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:06:42 2020 +0200

    Make value_sanitizer an argument of JournalWriter.
    
    Instead of being a method of KafkaJournalWriter.
    
    This will allow using JournalWriters for values that aren't model objects.

commit 1a64c96f97d8b6a56a873a269af88859083359ea
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 14:15:35 2020 +0200

    Remove swh.journal.serializers.object_key, use BaseModel.unique_key instead.

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

Harbormaster failed remote builds in B15695: Diff 14401!Sep 29 2020, 3:59 PM
vlorentz updated this revision to Diff 14928.Oct 12 2020, 12:19 PM

rebase

vlorentz planned changes to this revision.Oct 12 2020, 12:19 PM
swh-public-ci added a comment.Oct 12 2020, 12:22 PM

Build has FAILED

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

Harbormaster failed remote builds in B16160: Diff 14928!Oct 12 2020, 12:22 PM
vlorentz mentioned this in D4081: backfill: use get_journal_writer instead if instantiating JournalWriter directly..Oct 12 2020, 3:02 PM
vlorentz updated this revision to Diff 15206.Oct 19 2020, 2:02 PM

rebase

vlorentz retitled this revision from [WIP] Make the type of values of JournalWriter generic, so it works with types not from swh-model. to Make the type of values of JournalWriter generic, so it works with types not from swh-model..Oct 19 2020, 2:02 PM
swh-public-ci added a comment.Oct 19 2020, 2:05 PM

Build is green

Patch application report for D4082 (id=15206)

Could not rebase; Attempt merge onto abc42e2bf7...

Updating abc42e2..671c5c5
Fast-forward
 swh/journal/serializers.py             | 31 +--------------------
 swh/journal/tests/journal_data.py      |  4 +--
 swh/journal/tests/test_kafka_writer.py | 17 ++++++++----
 swh/journal/writer/__init__.py         | 29 ++++++++++++++++++++
 swh/journal/writer/inmemory.py         | 20 ++++++++------
 swh/journal/writer/kafka.py            | 49 +++++++++++++++++++---------------
 6 files changed, 83 insertions(+), 67 deletions(-)
Changes applied before test
commit 671c5c5158d8094c2861bfcba5096f7ea9d2ab30
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:58:07 2020 +0200

    Make the type of values of JournalWriter generic, so it works with types not from swh-model.
    
    The indexer-storage will use this.

commit efd94b05bebbfc082227719ac94b2223723f95ed
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:06:42 2020 +0200

    Make value_sanitizer an argument of JournalWriter.
    
    Instead of being a method of KafkaJournalWriter.
    
    This will allow using JournalWriters for values that aren't model objects.

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

Harbormaster completed remote builds in B16442: Diff 15206.Oct 19 2020, 2:05 PM
douardda added a subscriber: douardda.Oct 22 2020, 11:43 AM

Wouldn't it make a bit easier to name the generic version of the journal writer something like GenericKafkaJournalWriter and have KafkaJournalWriter = GenericKafkaJournalWriter[BaseModel] ? (for bw compat)

douardda added inline comments.Oct 22 2020, 11:46 AM
swh/journal/writer/kafka.py
67–68

The docstring should be updated to document the generic aspect of it.

vlorentz added a comment.Oct 22 2020, 1:14 PM
In D4082#107582, @douardda wrote:

Wouldn't it make a bit easier to name the generic version of the journal writer something like GenericKafkaJournalWriter and have KafkaJournalWriter = GenericKafkaJournalWriter[BaseModel] ? (for bw compat)

Why? This change won't break any code using KafkaJournalWriter

vlorentz mentioned this in D4080: Make value_sanitizer an argument of JournalWriter..Oct 22 2020, 1:20 PM
ardumont accepted this revision.Oct 22 2020, 2:29 PM
ardumont added a subscriber: ardumont.

Oh i missed that diff...

That's nice ;)

This revision is now accepted and ready to land.Oct 22 2020, 2:29 PM
vlorentz added a task: T2651: Make the indexer-storage publish its rows to Kafka.Oct 26 2020, 9:31 AM
olasd added a subscriber: olasd.Oct 26 2020, 12:12 PM

Can you document the generic type argument like @douardda asked?

douardda added a comment.Oct 26 2020, 12:25 PM
In D4082#107701, @vlorentz wrote:
In D4082#107582, @douardda wrote:

Wouldn't it make a bit easier to name the generic version of the journal writer something like GenericKafkaJournalWriter and have KafkaJournalWriter = GenericKafkaJournalWriter[BaseModel] ? (for bw compat)

Why? This change won't break any code using KafkaJournalWriter

Looks like the only user of the KafkaJournalWriter class outside the journal is swh.objstorage.replayer, which would be impacted by this diff (unless I'm mistaken) but this is indeed a minor issue.

vlorentz added a comment.Oct 26 2020, 12:35 PM
In D4082#108323, @douardda wrote:

Looks like the only user of the KafkaJournalWriter class outside the journal is swh.objstorage.replayer, which would be impacted by this diff (unless I'm mistaken) but this is indeed a minor issue.

There is no impact. This diff only extends how KafkaJournalWriter can be used, by accepting more types. All existing uses remain valid.

swh-objstorage-replayer only uses it in a test to do that:

writer = KafkaJournalWriter(
    brokers=[kafka_server],
    client_id="kafka_writer",
    prefix=kafka_prefix,
    anonymize=False,
)

for content in CONTENTS:
    objstorage1.add(content.data)
    writer.write_addition("content", content)
vlorentz added a comment.Oct 26 2020, 12:37 PM

Oh, I guess you meant replacing it to use writer = KafkaJournalWriter[BaseModel](, right?

That's optional, to give an extra type hint to mypy. Without [BaseModel], it just means mypy has to guess it. (It's like using List instead of List[foo])

vlorentz updated this revision to Diff 15547.Nov 2 2020, 9:29 AM

update docstring of KafkaJournalWriter.

vlorentz marked an inline comment as done.Nov 2 2020, 9:29 AM
swh-public-ci added a comment.Nov 2 2020, 9:31 AM

Build is green

Patch application report for D4082 (id=15547)

Could not rebase; Attempt merge onto e998407120...

Auto-merging swh/journal/tests/journal_data.py
Merge made by the 'recursive' strategy.
 swh/journal/serializers.py             | 31 +---------------
 swh/journal/tests/journal_data.py      |  4 +--
 swh/journal/tests/test_kafka_writer.py | 17 ++++++---
 swh/journal/writer/__init__.py         | 29 +++++++++++++++
 swh/journal/writer/inmemory.py         | 20 ++++++-----
 swh/journal/writer/kafka.py            | 65 ++++++++++++++++++++--------------
 6 files changed, 94 insertions(+), 72 deletions(-)
Changes applied before test
commit 380e19c21d574c5d2d95fe159c2e15769b688b6f
Merge: e998407 f3435ab
Author: Jenkins user <jenkins@localhost>
Date:   Mon Nov 2 08:29:33 2020 +0000

    Merge branch 'diff-target' into HEAD

commit f3435ab3be9f64b80848503ae4342e84b94d5a6f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:58:07 2020 +0200

    Make the type of values of JournalWriter generic, so it works with types not from swh-model.
    
    The indexer-storage will use this.

commit efd94b05bebbfc082227719ac94b2223723f95ed
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:06:42 2020 +0200

    Make value_sanitizer an argument of JournalWriter.
    
    Instead of being a method of KafkaJournalWriter.
    
    This will allow using JournalWriters for values that aren't model objects.

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

Harbormaster completed remote builds in B16777: Diff 15547.Nov 2 2020, 9:31 AM
douardda accepted this revision.Nov 2 2020, 12:06 PM
vlorentz updated this revision to Diff 15550.Nov 2 2020, 12:21 PM

rebase

swh-public-ci added a comment.Nov 2 2020, 12:25 PM

Build is green

Patch application report for D4082 (id=15550)

Could not rebase; Attempt merge onto e998407120...

Updating e998407..1a6c825
Fast-forward
 swh/journal/serializers.py             | 31 +---------------
 swh/journal/tests/journal_data.py      |  4 +--
 swh/journal/tests/test_kafka_writer.py | 17 ++++++---
 swh/journal/writer/__init__.py         | 29 +++++++++++++++
 swh/journal/writer/inmemory.py         | 20 ++++++-----
 swh/journal/writer/kafka.py            | 65 ++++++++++++++++++++--------------
 6 files changed, 94 insertions(+), 72 deletions(-)
Changes applied before test
commit 1a6c825884f383967811c613fc514507050da342
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:58:07 2020 +0200

    Make the type of values of JournalWriter generic, so it works with types not from swh-model.
    
    The indexer-storage will use this.

commit 3ae9a5bab45970217d4cd93785c47ffcd936ed57
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Sep 29 15:06:42 2020 +0200

    Make value_sanitizer an argument of JournalWriter.
    
    Instead of being a method of KafkaJournalWriter.
    
    This will allow using JournalWriters for values that aren't model objects.

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

Harbormaster completed remote builds in B16780: Diff 15550.Nov 2 2020, 12:25 PM
Closed by commit rDJNL1a6c825884f3: Make the type of values of JournalWriter generic, so it works with types not… (authored by vlorentz). · Explain WhyNov 2 2020, 12:41 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDJNL1a6c825884f3: Make the type of values of JournalWriter generic, so it works with types not….

Revision Contents
Changeset List

PathSize
swh/
journal/
31 lines
tests/
4 lines
8 lines
writer/
19 lines
19 lines
50 lines

Diff 15556

View Options

swh/journal/serializers.py

Loading...
View Options

swh/journal/tests/journal_data.py

Loading...
View Options

swh/journal/tests/test_kafka_writer.py

Loading...
View Options

swh/journal/writer/__init__.py

Loading...
View Options

swh/journal/writer/inmemory.py

Loading...
View Options

swh/journal/writer/kafka.py

Loading...
About · Developer information · Legal