⚙ D4641 test_journal_client: Send production objects to journal for testing

ardumont created this revision.Dec 2 2020, 9:24 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptDec 2 2020, 9:24 AM

Build has FAILED

Patch application report for D4641 (id=16471)

Could not rebase; Attempt merge onto 28ae49da34...

Updating 28ae49d..23a036a
Fast-forward
 conftest.py                               |   6 +-
 requirements-swh.txt                      |   2 +-
 swh/indexer/storage/db.py                 |   4 +-
 swh/indexer/tests/conftest.py             |  76 ++++++---
 swh/indexer/tests/test_cli.py             | 275 ++++++++++++++++++++----------
 swh/indexer/tests/test_journal_client.py  | 172 +++++++++----------
 swh/indexer/tests/test_origin_head.py     |  34 ++--
 swh/indexer/tests/test_origin_metadata.py |  89 +++++++---
 8 files changed, 395 insertions(+), 263 deletions(-)

Changes applied before test

commit 23a036ad20bd88b1f15e469e06fb01d0523424dd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    wip: test_journal_client: Send production objects to journal for testing
    
    It's broken for now as somehow only 1 object pass through the pipe and I don't
    get why so far.
    
    I also tried to use our real journal writer instead of the plain Producer so it
    match even more the reality of this test but that just hangs. So I reverted
    because, one problem at a time...
    
    Related to T2821

commit 305b265359e503459b78ec6de43b64fb0b31a511
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 1 17:55:36 2020 +0100

    test_journal_client: Migrate away from mocks
    
    Related to D4638

commit 394576b95688b867a86a5bcb4da30534640a0577
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Dec 1 15:40:01 2020 +0100

    tests: Use production backends within the indexer tests
    
    This detected some paper cuts within cli tests for example.
    
    The main goal is to decrease friction when actually deploying indexer related
    services (backend, indexers, ...).
    
    The pg backends tests should still be reasonably fast as it's using the
    swh.core.db.pytest_plugin (which truncate tables in between tests).
    
    Related to T2821

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

Harbormaster failed remote builds in B17657: Diff 16471!Dec 2 2020, 9:27 AM

fwiw, i tried to also use other producer.produce method to no avail [1]

[1] https://docs.confluent.io/2.0.0/clients/librdkafka/classRdKafka_1_1Producer.html#ac51872fe6252093fc0c271557d132d28

ardumont edited the summary of this revision. (Show Details)Dec 2 2020, 9:36 AM

ardumont added a parent revision: D4640: test_journal_client: Migrate away from mocks.

When I suggested this, I figured this would be configuring swh_storage to use a journal writer, rather than reimplementing that yourself.

producer.produce is asynchronous; You need to wait for the delivery callback before being able to check the messages at the other end of the pipeline. There's a good chance the client exhausts the messages available before they're actually all sent.

When I suggested this, I figured this would be configuring swh_storage to use a journal writer, rather than reimplementing that yourself.

That did not occur to me. Interesting.

I'll check that.
Thanks.

When I suggested this, I figured this would be configuring swh_storage to use a journal writer, rather than reimplementing that yourself.

That's actually annoying... create an origin-visit-status requires to create the origin visits first which requires to create the origins...

I actually used a journal writer nonetheless (independently from a storage), it no longer hangs [1] as initially said in the diff description.

That's still finding only 1 message as well though...

so...

producer.produce is asynchronous; You need to wait for the delivery callback before being able to check the messages at the other end of the pipeline. There's a good chance the client exhausts the messages available before they're actually all sent.

I guess this holds true for the journal writer as well.
I'll update the diff with that checkout so it's actually demonstrated.

[1] hang solved: maybe the input in the get_journal_writer was incorrect (broker expected a list of brokers, not a single string...).

test_cli: Use a journal_writer instead of a plain Producer

Build has FAILED

Patch application report for D4641 (id=16519)

Rebasing onto f775fea12b...

Current branch diff-target is up to date.

Changes applied before test

commit 133de321ead3e33c9cdfd45cc2ceca5ca7702c97
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Dec 3 15:56:43 2020 +0100

    test_cli: Use a journal_writer instead of a plain Producer
    
    It's able to deal with swh.model.model objects.

commit 84e2ade7612d974aff977a77bdc7f8cbe5cf8f8d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    wip: test_journal_client: Send production objects to journal for testing
    
    It's broken for now as somehow only 1 object pass through the pipe and I don't
    get why so far.
    
    I also tried to use our real journal writer instead of the plain Producer so it
    match even more the reality of this test but that just hangs. So I reverted
    because, one problem at a time...
    
    Related to T2821

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

Harbormaster failed remote builds in B17705: Diff 16519!Dec 3 2020, 4:01 PM

I'll check more properly how those are tested in the actual journal... There might be hints there.

Thanks @vlorentz which pointed out that there is one or more tasks (undeterministically).
The scheduled task arguments contains the origin-visit-status which are grouped together...

So the test need to be reworked a tad.

test_cli_journal_client: Rework assertions

ardumont edited the summary of this revision. (Show Details)Dec 4 2020, 1:30 PM

ardumont edited the test plan for this revision. (Show Details)

vlorentz requested changes to this revision.Dec 4 2020, 1:35 PM

vlorentz added inline comments.

swh/indexer/tests/test_cli.py
477	this makes the test succeed even if there are no tasks at all. Why not an equality?

This revision now requires changes to proceed.Dec 4 2020, 1:35 PM

ardumont added inline comments.Dec 4 2020, 1:37 PM

swh/indexer/tests/test_cli.py
477	ah i forgot to deal with the non-deterministic part... i'll check that ... note that currently, given the celery stuff issue, tests will timeout...

Build was aborted

Patch application report for D4641 (id=16554)

Rebasing onto f775fea12b...

Current branch diff-target is up to date.

Changes applied before test

commit 449ce394bb3866a9093b4be8f1451550f14c7210
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Dec 4 13:26:18 2020 +0100

    test_cli_journal_client: Rework assertions

commit 133de321ead3e33c9cdfd45cc2ceca5ca7702c97
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Dec 3 15:56:43 2020 +0100

    test_cli: Use a journal_writer instead of a plain Producer
    
    It's able to deal with swh.model.model objects.

commit 84e2ade7612d974aff977a77bdc7f8cbe5cf8f8d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    wip: test_journal_client: Send production objects to journal for testing
    
    It's broken for now as somehow only 1 object pass through the pipe and I don't
    get why so far.
    
    I also tried to use our real journal writer instead of the plain Producer so it
    match even more the reality of this test but that just hangs. So I reverted
    because, one problem at a time...
    
    Related to T2821

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

Harbormaster failed remote builds in B17738: Diff 16554!Dec 4 2020, 1:48 PM

Adapt according to review

Build was aborted

Patch application report for D4641 (id=16557)

Rebasing onto f775fea12b...

Current branch diff-target is up to date.

Changes applied before test

commit 5644ba47267266600927905d94057804ee978a50
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Dec 4 13:26:18 2020 +0100

    test_cli_journal_client: Rework assertions

commit 133de321ead3e33c9cdfd45cc2ceca5ca7702c97
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Dec 3 15:56:43 2020 +0100

    test_cli: Use a journal_writer instead of a plain Producer
    
    It's able to deal with swh.model.model objects.

commit 84e2ade7612d974aff977a77bdc7f8cbe5cf8f8d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    wip: test_journal_client: Send production objects to journal for testing
    
    It's broken for now as somehow only 1 object pass through the pipe and I don't
    get why so far.
    
    I also tried to use our real journal writer instead of the plain Producer so it
    match even more the reality of this test but that just hangs. So I reverted
    because, one problem at a time...
    
    Related to T2821

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

Harbormaster failed remote builds in B17740: Diff 16557!Dec 4 2020, 2:16 PM

vlorentz accepted this revision.Dec 4 2020, 3:19 PM

vlorentz added inline comments.

swh/indexer/tests/test_cli.py
478–481	remove `assert origin in set([vs.origin for vs in visit_statuses_full])` and replace `assert len(actual_origins) == len(visit_statuses_full)` with `assert actual_origins == {vs.origin for vs in visit_statuses_full}` it's equivalent but simpler.

This revision is now accepted and ready to land.Dec 4 2020, 3:19 PM

Adapt according to last suggestion (neat :)
Rework commit messages (squash and adapt message)

ardumont marked an inline comment as done.Dec 4 2020, 3:41 PM

ardumont added inline comments.

swh/indexer/tests/test_cli.py
478–481	neat ;)

Build was aborted

Patch application report for D4641 (id=16562)

Rebasing onto f775fea12b...

Current branch diff-target is up to date.

Changes applied before test

commit e0f1199c210245fab00e12136a49295c902b3efc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    test_journal_client_cli: Send production objects to journal
    
    The reason for this is to avoid surprises like the indexer journal client stuck
    in limbo for a while.
    
    Related to T2821
    Related to T2814

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

Harbormaster failed remote builds in B17745: Diff 16562!Dec 4 2020, 4:01 PM

ardumont marked an inline comment as done.Dec 4 2020, 5:00 PM

Build was aborted

Patch application report for D4641 (id=16562)

Rebasing onto f775fea12b...

Current branch diff-target is up to date.

Changes applied before test

commit e0f1199c210245fab00e12136a49295c902b3efc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    test_journal_client_cli: Send production objects to journal
    
    The reason for this is to avoid surprises like the indexer journal client stuck
    in limbo for a while.
    
    Related to T2821
    Related to T2814

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

Build is green

Patch application report for D4641 (id=16562)

Rebasing onto f775fea12b...

Current branch diff-target is up to date.

Changes applied before test

commit e0f1199c210245fab00e12136a49295c902b3efc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Dec 2 09:21:51 2020 +0100

    test_journal_client_cli: Send production objects to journal
    
    The reason for this is to avoid surprises like the indexer journal client stuck
    in limbo for a while.
    
    Related to T2821
    Related to T2814

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

Harbormaster completed remote builds in B17745: Diff 16562.Dec 7 2020, 10:50 AM

Closed by commit rDCIDXe0f1199c2102: test_journal_client_cli: Send production objects to journal (authored by ardumont). · Explain WhyDec 7 2020, 10:51 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDCIDXe0f1199c2102: test_journal_client_cli: Send production objects to journal.

test_journal_client: Send production objects to journal for testing
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D4641 (id=16471)

Changes applied before test

Patch application report for D4641 (id=16519)

Changes applied before test

Patch application report for D4641 (id=16554)

Changes applied before test

Patch application report for D4641 (id=16557)

Changes applied before test

Patch application report for D4641 (id=16562)

Changes applied before test

Patch application report for D4641 (id=16562)

Changes applied before test

Patch application report for D4641 (id=16562)

Changes applied before test

Revision Contents
Changeset List

Diff 16584

swh/indexer/tests/test_cli.py

test_journal_client: Send production objects to journal for testingClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D4641 (id=16471)

Changes applied before test

Patch application report for D4641 (id=16519)

Changes applied before test

Patch application report for D4641 (id=16554)

Changes applied before test

Patch application report for D4641 (id=16557)

Changes applied before test

Patch application report for D4641 (id=16562)

Changes applied before test

Patch application report for D4641 (id=16562)

Changes applied before test

Patch application report for D4641 (id=16562)

Changes applied before test

Revision ContentsChangeset List

Diff 16584

swh/indexer/tests/test_cli.py

test_journal_client: Send production objects to journal for testing
ClosedPublic
Actions

Revision Contents
Changeset List