Page MenuHomeSoftware Heritage

origin_visit_upsert: Use OriginVisit objects as input
ClosedPublic

Authored by ardumont on Mar 11 2020, 5:38 PM.

Details

Summary

This aligns with other _add endpoints [1]

Impacts:

  • D2826: swh-journal (replayer)

Related to D2812#67298

[1] except for origin_visit_add which is next (D2820)

Test Plan

tox

Diff Detail

Repository
rDSTO Storage manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Mar 11 2020, 5:38 PM
ardumont edited the summary of this revision. (Show Details)Mar 12 2020, 2:22 AM
ardumont edited the summary of this revision. (Show Details)
ardumont edited the summary of this revision. (Show Details)Mar 12 2020, 10:45 AM
vlorentz requested changes to this revision.Mar 12 2020, 2:27 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/storage/cassandra/storage.py
830

This shouldn't type-check.

Call json.dumps in CqlRunner.origin_visit_upsert, like CqlRunner.origin_visit_update does.

swh/storage/db.py
382

Do we want swh-model objects in db.py?

This revision now requires changes to proceed.Mar 12 2020, 2:27 PM
ardumont added inline comments.Mar 12 2020, 2:44 PM
swh/storage/db.py
382

why not?

vlorentz added inline comments.Mar 12 2020, 3:10 PM
swh/storage/db.py
382

Because we didn't do it so far, so this introduces an inconsistency.

ardumont added inline comments.Mar 12 2020, 6:23 PM
swh/storage/db.py
382

but we are trying to fix that inconsistency everywhere by using model object...
I'm trying to get there...

ardumont added inline comments.Mar 12 2020, 6:24 PM
swh/storage/cassandra/storage.py
830

ok, will do.

ardumont edited the summary of this revision. (Show Details)Mar 12 2020, 6:24 PM
ardumont added inline comments.Mar 12 2020, 6:28 PM
swh/storage/db.py
382

also you typed cql with base model object
This cql (cassandra) module is the equivalent of the db (pg) here...
So that's the consistent way forward.

vlorentz added inline comments.Mar 12 2020, 6:38 PM
swh/storage/db.py
382

Sure, but I would rather this single function is consistent with the rest of its module, than with a completely different module.

But if we want to make db.py use model objects, then sure

ardumont added inline comments.Mar 12 2020, 6:41 PM
swh/storage/cassandra/storage.py
830

This shouldn't type-check.

metadata = attr.ib(type=Optional[Dict[str, object]],
                   default=None)

I see now. json.dumps could return a string, list or Dict...

Well, that's actually a field which is not used...

ardumont added inline comments.Mar 12 2020, 6:46 PM
swh/storage/cassandra/storage.py
830

json.dumps could return a string, list or Dict...

no, just a string...
ok i see (pfff, needs some nap)

ardumont updated this revision to Diff 10038.Mar 12 2020, 7:09 PM

Adapt according to review

ardumont edited the summary of this revision. (Show Details)Mar 12 2020, 8:21 PM
ardumont added inline comments.Mar 13 2020, 10:01 AM
swh/storage/db.py
382

Well consistency is the goal.
But i'm doing that incrementally as i go along.

This revision is now accepted and ready to land.Mar 13 2020, 11:31 AM
This revision was automatically updated to reflect the committed changes.
ardumont edited the summary of this revision. (Show Details)Mar 14 2020, 11:42 AM