Page MenuHomeSoftware Heritage
Differential D8705

writer: make calling flush() in write_addition(s)() optional
ClosedPublic
Actions

Authored by douardda on Oct 18 2022, 4:20 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDJNL7c83ee11ecab: writer: make calling flush() in write_addition(s)() optional
Summary

using a constructor 'auto_flush' bookean argument.
The idea is that in a test session, each call to 'flush()' takes ~1s to
run, so having the test handling a single call to flush() when needed
(instead of n calls) make test execution significantly faster.

For example, swh-storage's test_replay.py execution went from ~140s to
~40s, and test_backfill.py from ~40s to ~15s.

Depends on D8697

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

douardda created this revision.Oct 18 2022, 4:20 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptOct 18 2022, 4:20 PM
swh-public-ci added a comment.Oct 18 2022, 4:20 PM

Build has FAILED

Patch application report for D8705 (id=31427)

Could not rebase; Attempt merge onto 1f1b03cfbd...

Updating 1f1b03c..a0e2a57
Fast-forward
 swh/journal/pytest_plugin.py | 2 +-
 swh/journal/writer/kafka.py  | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)
Changes applied before test
commit a0e2a57813221d9a13d44a00a73c7df53d32180c
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 15:34:31 2022 +0200

    writer: make calling flush() in write_addition(s)() optional
    
    using a constructor 'auto_flush' bookean argument.
    The idea is that in a test session, each call to 'flush()' takes ~1s to
    run, so having the test handling a single call to flush() when needed
    (instead of n calls) make test execution significantly faster.
    
    For example, swh-storage's test_replay.py execution went from ~140s to
    ~40s, and test_backfill.py from ~40s to ~15s.

commit eaf6c57a8407851d27f4acd94bd8bd01bc003841
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 12:03:58 2022 +0200

    tests: make the kafka_server_base fixture a function scoped one
    
    this actually speeds up tests quite a bit (preventing a 30s timeout when
    the fixture is actually reused several times in a test session).

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

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 18 2022, 4:20 PM
Harbormaster failed remote builds in B32372: Diff 31427!
douardda updated this revision to Diff 31499.Oct 18 2022, 8:03 PM

rebase

douardda mentioned this in D8740: tests: only flush() the kafka journal writer once per test.Oct 18 2022, 8:07 PM
swh-public-ci added a comment.Oct 18 2022, 8:19 PM

Build is green

Patch application report for D8705 (id=31499)

Could not rebase; Attempt merge onto 2c6d9e05ec...

Updating 2c6d9e0..887a93e
Fast-forward
 swh/journal/pytest_plugin.py | 2 +-
 swh/journal/writer/kafka.py  | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)
Changes applied before test
commit 887a93e99c5f85cf1169873ed3e98819fd644f00
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 15:34:31 2022 +0200

    writer: make calling flush() in write_addition(s)() optional
    
    using a constructor 'auto_flush' bookean argument.
    The idea is that in a test session, each call to 'flush()' takes ~1s to
    run, so having the test handling a single call to flush() when needed
    (instead of n calls) make test execution significantly faster.
    
    For example, swh-storage's test_replay.py execution went from ~140s to
    ~40s, and test_backfill.py from ~40s to ~15s.

commit 04a84dba6a175a509d6f8d4ccde1441b8e352858
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 12:03:58 2022 +0200

    tests: make the kafka_server_base fixture a function scoped one
    
    this actually speeds up tests quite a bit (preventing a 30s timeout when
    the fixture is actually reused several times in a test session).

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

Harbormaster completed remote builds in B32445: Diff 31499.Oct 18 2022, 8:19 PM
douardda requested review of this revision.Oct 18 2022, 8:19 PM
ardumont added a subscriber: ardumont.Oct 20 2022, 11:09 AM

If it's for test speedup only, don't you want to keep the actual behavior instead?
That is, make the auto_flush be False by default, and then you explicitely set it up to True in the calling tests.

olasd accepted this revision.Oct 20 2022, 11:15 AM
olasd added a subscriber: olasd.
In D8705#227328, @ardumont wrote:

If it's for test speedup only, don't you want to keep the actual behavior instead?
That is, make the auto_flush be False by default, and then you explicitely set it up to True in the calling tests.

I believe that auto_flush=True is the current default, so the diff does what you're asking :-)

I'd be keen on having a warning, at least when we __del__ the object and some deliveries are still pending, because even if not perfect, that'd be the sign of a bug.

This revision is now accepted and ready to land.Oct 20 2022, 11:15 AM
ardumont added a comment.Oct 20 2022, 11:22 AM

If it's for test speedup only, don't you want to keep the actual behavior instead?
That is, make the auto_flush be False by default, and then you explicitely set it up to True in the calling tests.

I believe that auto_flush=True is the current default, so the diff does what you're asking :-)

I'd be keen on having a warning, at least when we __del__ the object and some deliveries are still pending, because even if not perfect, that'd be the sign of a bug.

ok, that'd also make sense... but then how come there is any speed improvments here?
I mean, I don't see any changes in tests saying then auto_flush=False in that case...

...ah... maybe, it's for other tests in another repository, right?

olasd added a subscriber: vlorentz.Oct 20 2022, 11:48 AM

@vlorentz mentions that this is missing a docstring change

olasd added a comment.Oct 20 2022, 11:50 AM
In D8705#227333, @ardumont wrote:

...ah... maybe, it's for other tests in another repository, right?

Yeah. swh-storage and swh-journal will be affected, as they call write_addition(s) on multiple object types one after the other, and have to wait for each round. And it seems that the mock broker adds a 1 second delay for flushes, for some reason.

douardda updated this revision to Diff 31537.Oct 21 2022, 4:19 PM

Add the missing docstring entry

swh-public-ci added a comment.Oct 21 2022, 4:22 PM

Build is green

Patch application report for D8705 (id=31537)

Could not rebase; Attempt merge onto 2c6d9e05ec...

Updating 2c6d9e0..1f607fb
Fast-forward
 swh/journal/pytest_plugin.py |  2 +-
 swh/journal/writer/kafka.py  | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)
Changes applied before test
commit 1f607fb558659123db92b0a5c93efcdf0d64ab00
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 15:34:31 2022 +0200

    writer: make calling flush() in write_addition(s)() optional
    
    using a constructor 'auto_flush' bookean argument.
    The idea is that in a test session, each call to 'flush()' takes ~1s to
    run, so having the test handling a single call to flush() when needed
    (instead of n calls) make test execution significantly faster.
    
    For example, swh-storage's test_replay.py execution went from ~140s to
    ~40s, and test_backfill.py from ~40s to ~15s.

commit 04a84dba6a175a509d6f8d4ccde1441b8e352858
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 12:03:58 2022 +0200

    tests: make the kafka_server_base fixture a function scoped one
    
    this actually speeds up tests quite a bit (preventing a 30s timeout when
    the fixture is actually reused several times in a test session).

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

Harbormaster completed remote builds in B32491: Diff 31537.Oct 21 2022, 4:22 PM
douardda updated this revision to Diff 31539.Oct 21 2022, 4:26 PM

typos

swh-public-ci added a comment.Oct 21 2022, 4:30 PM

Build is green

Patch application report for D8705 (id=31539)

Could not rebase; Attempt merge onto 2c6d9e05ec...

Updating 2c6d9e0..7c83ee1
Fast-forward
 swh/journal/pytest_plugin.py |  2 +-
 swh/journal/writer/kafka.py  | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)
Changes applied before test
commit 7c83ee11ecab206e082e06aaa0b8330ef6677d2c
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 15:34:31 2022 +0200

    writer: make calling flush() in write_addition(s)() optional
    
    using a constructor 'auto_flush' bookean argument.
    The idea is that in a test session, each call to 'flush()' takes ~1s to
    run, so having the test handling a single call to flush() when needed
    (instead of n calls) make test execution significantly faster.
    
    For example, swh-storage's test_replay.py execution went from ~140s to
    ~40s, and test_backfill.py from ~40s to ~15s.

commit 04a84dba6a175a509d6f8d4ccde1441b8e352858
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Oct 18 12:03:58 2022 +0200

    tests: make the kafka_server_base fixture a function scoped one
    
    this actually speeds up tests quite a bit (preventing a 30s timeout when
    the fixture is actually reused several times in a test session).

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

Harbormaster completed remote builds in B32493: Diff 31539.Oct 21 2022, 4:30 PM
Closed by commit rDJNL7c83ee11ecab: writer: make calling flush() in write_addition(s)() optional (authored by douardda). · Explain WhyOct 21 2022, 4:31 PM
This revision was automatically updated to reflect the committed changes.
douardda added a commit: rDJNL7c83ee11ecab: writer: make calling flush() in write_addition(s)() optional.

Revision Contents
Changeset List

PathSize
swh/
journal/
writer/
11 lines

Diff 31541

View Options

swh/journal/writer/kafka.py

Loading...
About · Developer information · Legal