Details

Reviewers

Group Reviewers

Commits

Summary

service that listen to the 'swh.journal.objects.content' topic and replicate object blobs from a source object storage to a destination one.

refactor replayer tests; implement the whole kafka mocking logic in the Mocked* classes and delete the test_write_replay_same_order which was not testing anything more than what the orther test do.

client: simplify a bit the JournalClient class no need for those poll() and commit() proxy methods.

Depends on D1540

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.

vlorentz requested changes to this revision.Jun 5 2019, 12:27 PM

vlorentz added a subscriber: vlorentz.

vlorentz added inline comments.

swh/journal/cli.py
166–178	I think these error messages should tell the exact name of the missing key, so it's easier to look for them in an example config file. (And we should also have an example config file)
180–184	We should raise an error if there is no prefix or group id.
swh/journal/client.py
81–86 ↗	(On Diff #5076)	These methods are mocked in the tests.
swh/journal/tests/test_write_replay.py
31	A writer is not a consumer. You would want to use `MockConsumer` instead.
43–48	not needed in a writer. You should put that in a new class, eg. `MockConsumer`.
70	same

This revision now requires changes to proceed.Jun 5 2019, 12:27 PM

swh/journal/cli.py
180–184	not for the prefix, since the default value is fine (and is the one that will be used in most of the cases... TBH, I don't really see the reason this is configurable). `group_id` is indeed a mandatory config option and should not have a default value...
swh/journal/client.py
81–86 ↗	(On Diff #5076)	not any more :-)
swh/journal/tests/test_write_replay.py
31	as said before, this mockup is not a nice mockup of a kafka consumer nor a kafka producer, but some sort of frankenstein of both. It does the job, but is not so pretty.
43–48	this mock is both a writer and a consumer in this test. I mostly just merged several sparse functions in this class. I was not very pleased with the way these tests were written before, so I did this, but the result does not please me too much neither...

Also, your new CLI needs tests :)

swh/journal/cli.py
180–184	if `prefix is None` and there is no prefix in the config, then `prefix` will be set to `None`, not to DEFAULT_PREFIX. And we don't want that.
swh/journal/client.py
81–86 ↗	(On Diff #5076)	I forgot to remove that comment after actually reading the diff, sorry
swh/journal/tests/test_write_replay.py
43–48	this mock is both a writer and a consumer in this test these are two different things, so they should be different classes. but the result does not please me too much neither... Did you try using `unittest.mock.patch`?

swh/journal/cli.py
180–184	indeed we do not want that. I did write the correct version of this stuff at some point but I guess I messed during a rebase or smthg
swh/journal/tests/test_write_replay.py
43–48	please note I "just" refactored existing tests...

should address vlortenz's comments

fix commit msg

Build has FAILED

Build has FAILED

swh/journal/replay.py
46–49	inconsistent with the behavior of `_insert_objects` (which asserts False). I have a preference to using the existing behavior (crashing) for both instead of just logging, but that's up to you

swh/journal/replay.py
46–49	Thinking about it, it's probably better to log. Could you raise the log level to warning (so it doesn't go unnoticed), and make `_insert_objects` do that too?