Page MenuHomeSoftware Heritage
Differential D1294

Add a new JournalWriter interface, which is notified by swh-storage before writing to pgsql.
ClosedPublic
Actions

Authored by vlorentz on Mar 26 2019, 4:50 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T1600: Write a storage backend that writes to kafka
Commits
rDSTOC246855c813a2: Add a new JournalWriter interface, which is notified by swh-storage before…
R65:246855c813a2: Add a new JournalWriter interface, which is notified by swh-storage before…
rDSTO246855c813a2: Add a new JournalWriter interface, which is notified by swh-storage before…

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

vlorentz created this revision.Mar 26 2019, 4:50 PM
swh-public-ci added a subscriber: swh-public-ci.Mar 26 2019, 4:55 PM

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

Harbormaster completed remote builds in B4839: Diff 4093.Mar 26 2019, 4:55 PM
vlorentz added a task: T1600: Write a storage backend that writes to kafka.Mar 26 2019, 5:10 PM
vlorentz updated this revision to Diff 4094.Mar 26 2019, 5:11 PM
  • remove debug print
swh-public-ci added a comment.Mar 26 2019, 5:15 PM

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

Harbormaster completed remote builds in B4840: Diff 4094.Mar 26 2019, 5:15 PM
vlorentz updated this revision to Diff 4097.Mar 27 2019, 10:41 AM
  • Add support for importing the future DirectKafkaWriter class from the journal
swh-public-ci added a comment.Mar 27 2019, 10:46 AM

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

Harbormaster completed remote builds in B4843: Diff 4097.Mar 27 2019, 10:46 AM
olasd requested changes to this revision.Mar 27 2019, 10:54 AM

I think all the self.assertEquals should be assertCountEquals (we don't really care about the order of objects there).

Is the id really needed in the journal writer interface? If so, the id of the objects should rather match what we're using in kafka (so, for contents, it's not just the sha1, it'd be a sorted tuple of all the hashes), but I think we can just drop it.

This revision now requires changes to proceed.Mar 27 2019, 10:54 AM
olasd added a comment.Mar 27 2019, 10:55 AM
In D1294#27689, @olasd wrote:

I think all the self.assertEquals should be assertCountEquals (we don't really care about the order of objects there).

Scratch that, we do care that origin_visit updates get applied in order here.

vlorentz added a comment.Mar 27 2019, 2:18 PM
In D1294#27689, @olasd wrote:

Is the id really needed in the journal writer interface?

Yes, for compaction.

If so, the id of the objects should rather match what we're using in kafka (so, for contents, it's not just the sha1, it'd be a sorted tuple of all the hashes)

will do

vlorentz added a comment.Mar 27 2019, 2:55 PM
In D1294#27689, @olasd wrote:

(so, for contents, it's not just the sha1, it'd be a sorted tuple of all the hashes)

According to both the publisher's code and the tests, it appears that the sha1 is the only key.

vlorentz updated this revision to Diff 4141.EditedMar 27 2019, 3:56 PM

drop id handling logic from the storage.

swh-public-ci added a comment.Mar 27 2019, 4:00 PM

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

Harbormaster completed remote builds in B4896: Diff 4141.Mar 27 2019, 4:00 PM
olasd requested changes to this revision.Mar 27 2019, 4:31 PM
  • the snapshot_add methods should actually record two changes:
    • the addition of the snapshot (with full data)
    • the update of the origin visit (with full data as well)
  • every write of a change to an origin visit (origin_visit_add, origin_visit_update *and* snapshot_add) should send the full data currently associated with the origin visit:
    • origin data
    • visit id
    • date
    • status
    • snapshot id
    • metadata

If we don't do that, we run the risk of clobbering existing data with an incomplete object.

This revision now requires changes to proceed.Mar 27 2019, 4:31 PM
vlorentz updated this revision to Diff 4144.EditedMar 27 2019, 4:52 PM
  • Use origin dictionaries instead of origin ids when dealing with origin_visits
  • On origin_visit updates, send all the data of the visit to the journal writer.
vlorentz planned changes to this revision.Mar 27 2019, 4:55 PM
swh-public-ci added a comment.Mar 27 2019, 4:57 PM

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

Harbormaster completed remote builds in B4899: Diff 4144.Mar 27 2019, 4:57 PM
vlorentz added a comment.Mar 27 2019, 5:03 PM
In D1294#28007, @olasd wrote:
  • the snapshot_add methods should actually record two changes:
    • the addition of the snapshot (with full data)
    • the update of the origin visit (with full data as well)

Should it really update the origin visit? A reader of the journal can infer that the origin_visit must be updated when reading the message in the snapshot topic.

vlorentz updated this revision to Diff 4146.Mar 27 2019, 5:37 PM

revert doc change

Harbormaster completed remote builds in B4901: Diff 4146.Mar 27 2019, 5:41 PM
swh-public-ci added a comment.Mar 27 2019, 5:41 PM

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

vlorentz planned changes to this revision.Mar 27 2019, 10:20 PM
In D1294#28022, @vlorentz wrote:
In D1294#28007, @olasd wrote:
  • the snapshot_add methods should actually record two changes:
    • the addition of the snapshot (with full data)
    • the update of the origin visit (with full data as well)

Should it really update the origin visit? A reader of the journal can infer that the origin_visit must be updated when reading the message in the snapshot topic.

Hmm... I didn't realize snapshot_add actually does two very different things: add a snapshot (which does not have an FK to the origin visit) AND update the origin_visit to point to that snapshot.

olasd added a comment.Mar 28 2019, 10:47 AM
In D1294#28022, @vlorentz wrote:

Hmm... I didn't realize snapshot_add actually does two very different things: add a snapshot (which does not have an FK to the origin visit) AND update the origin_visit to point to that snapshot.

Yeah, the method should probably be split in two ("add a snapshot", "update the origin_visit to record that it points to the snapshot"). This is an artefact of the old way occurrences were handled.

In any case, we're going to need that split when we write the code that reads all the graph leaves from kafka separately, and tries to store them back into PostgreSQL.

olasd added a comment.Mar 28 2019, 10:49 AM
In D1294#28133, @olasd wrote:
In D1294#28022, @vlorentz wrote:

Hmm... I didn't realize snapshot_add actually does two very different things: add a snapshot (which does not have an FK to the origin visit) AND update the origin_visit to point to that snapshot.

Yeah, the method should probably be split in two ("add a snapshot", "update the origin_visit to record that it points to the snapshot"). This is an artefact of the old way occurrences were handled.

That refactoring should be fairly easy as the only users of that method are:

  • swh.loader.core
  • the tests for the indexer
vlorentz updated this revision to Diff 4163.Mar 28 2019, 11:12 AM
  • Write origin_visit updates when a snapshot is added.
swh-public-ci added a comment.Mar 28 2019, 11:17 AM

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

Harbormaster completed remote builds in B4920: Diff 4163.Mar 28 2019, 11:17 AM
olasd accepted this revision.Mar 28 2019, 11:35 AM
This revision is now accepted and ready to land.Mar 28 2019, 11:35 AM
vlorentz updated this revision to Diff 4166.Mar 28 2019, 11:42 AM

rebase/squash

This revision was landed with ongoing or failed builds.Mar 28 2019, 11:42 AM
Closed by commit rDSTO246855c813a2: Add a new JournalWriter interface, which is notified by swh-storage before… (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
Harbormaster failed remote builds in B4922: Diff 4166!Mar 28 2019, 11:43 AM
swh-public-ci added a comment.Mar 28 2019, 11:43 AM

Build has FAILED

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

Revision Contents
Changeset List

PathSize
swh/
storage/
72 lines
34 lines
98 lines
tests/
4 lines
16 lines
5 lines
190 lines

Diff 4167

View Options

swh/storage/in_memory.py

Loading...
View Options

swh/storage/journal_writer.py

Loading...
View Options

swh/storage/storage.py

Loading...
View Options

swh/storage/tests/storage_testing.py

Loading...
View Options

swh/storage/tests/test_api_client.py

Loading...
View Options

swh/storage/tests/test_in_memory.py

Loading...
View Options

swh/storage/tests/test_storage.py

Loading...
About · Developer information · Legal