Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fast import: basic python test #10271

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

NanoBjorn
Copy link
Contributor

@NanoBjorn NanoBjorn commented Jan 3, 2025

We did not have any tests on fast_import binary yet.

In this PR I have introduced:

  • FastImport class and tools for testing in python
  • basic test that runs fast import against vanilla postgres and checks that data is there

Should be merged after #10251

@NanoBjorn NanoBjorn requested review from jcsp and problame January 3, 2025 14:43
@NanoBjorn NanoBjorn requested a review from a team as a code owner January 3, 2025 14:43
@NanoBjorn NanoBjorn requested review from tristan957 and removed request for a team January 3, 2025 14:44
from fixtures.neon_fixtures import VanillaPostgres, PgProtocol, PgBin
from fixtures.port_distributor import PortDistributor


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps makes sense to make this an extension to test_pgdata_import_smoke, which has this comment currently:

    # We have a Postgres data directory now.
    # Make a localfs remote storage that looks like how after `fast_import` ran.
    # TODO: actually exercise fast_import here

Copy link
Contributor Author

@NanoBjorn NanoBjorn Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought that it might not worth doing it all in the same test due to increased complexity -> more complicated debugging if something is failing. Right now it is using vanilla postgres pgdata as we do in fast import, so it should be enough for testing pageserver part.

Moved mine simple test to the same file, but will keep test_pgdata_import_smoke the same. Also added a todo to test full import flow separately, which will complement test_pgdata_import_smoke and test_fast_import_binary, but not complicate any of those.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with John that extending test_pgdata_import_smoke would be more valuable.
If I'm reading the TODO after your new test correctly, it would be completed by extending test_pgdata_import_smoke with what you have in your new test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed in DM that I will keep test_pgdata_import_smoke as is and add another one with s3 + pageserver + fast import binary, but if you don't mind I would do it in #10407 because it will anyway need something close

@NanoBjorn NanoBjorn requested a review from jcsp January 3, 2025 15:12
Copy link

github-actions bot commented Jan 3, 2025

7359 tests run: 6977 passed, 0 failed, 382 skipped (full report)


Flaky tests (6)

Postgres 17

Postgres 16

  • test_physical_replication_config_mismatch_max_locks_per_transaction: release-arm64

Postgres 15

Postgres 14

Code coverage* (full report)

  • functions: 33.6% (8432 of 25073 functions)
  • lines: 49.1% (70545 of 143652 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
fd09d31 at 2025-01-17T16:33:10.731Z :recycle:

@NanoBjorn NanoBjorn force-pushed the 22037-basic-fast-import-e2e branch 4 times, most recently from 122e324 to e856fae Compare January 6, 2025 19:36
@NanoBjorn NanoBjorn changed the title Basic fast_import python test fast import: basic python test Jan 7, 2025
@NanoBjorn NanoBjorn requested review from hlinnaka and arpad-m and removed request for tristan957 January 14, 2025 11:26
@NanoBjorn NanoBjorn force-pushed the 22100-change-fastimport-db-name branch from 73c14a7 to ebe26e2 Compare January 14, 2025 11:45
@NanoBjorn NanoBjorn force-pushed the 22037-basic-fast-import-e2e branch from e856fae to a80dcfa Compare January 14, 2025 11:45
@VladLazar VladLazar self-requested a review January 14, 2025 12:46
@NanoBjorn NanoBjorn removed request for jcsp and arpad-m January 14, 2025 13:09
Base automatically changed from 22100-change-fastimport-db-name to main January 15, 2025 20:52
@NanoBjorn NanoBjorn force-pushed the 22037-basic-fast-import-e2e branch from 01e969b to 047b986 Compare January 16, 2025 18:03
Comment on lines 109 to 114
let pg_port = if args.pg_port.is_some() {
args.pg_port.unwrap()
} else {
info!("pg_port not specified, using default 5432");
5432
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can match instead and avoid the unwrap

let pg_port = match args.pg_port {
  Some(port) => port,
  None {
    info!("...");
    5432
  }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to a little shorter version with unwrap_or_else

if extra_env is None:
env_vars = {}
else:
env_vars = extra_env.copy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why is the copy needed?

Copy link
Contributor Author

@NanoBjorn NanoBjorn Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra_env is passed as dict by reference, so it seems better to freeze it this way at the moment of initialization

basically copied from

if extra_env is None:
env_vars = {}
else:
env_vars = extra_env.copy()

from fixtures.neon_fixtures import VanillaPostgres, PgProtocol, PgBin
from fixtures.port_distributor import PortDistributor


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with John that extending test_pgdata_import_smoke would be more valuable.
If I'm reading the TODO after your new test correctly, it would be completed by extending test_pgdata_import_smoke with what you have in your new test.

@NanoBjorn NanoBjorn requested a review from VladLazar January 17, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants