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

test(pageserver): add test interface to create artificial layers #7899

Merged
merged 3 commits into from
May 31, 2024

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented May 28, 2024

Problem

This pull request adds necessary interfaces to deterministically create scenarios we want to test. Simplify some test cases to use this interface to make it stable + reproducible.

Compaction test will be able to use this interface. Also the upcoming delete tombstone tests will use this interface to make test reproducible.

Summary of changes

  • force_create_image_layer
  • force_create_delta_layer
  • force_advance_lsn
  • create_test_timeline_with_states
  • branch_timeline_test_with_states

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@skyzh skyzh requested a review from arpad-m May 28, 2024 20:47
@skyzh skyzh requested a review from a team as a code owner May 28, 2024 20:47
Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh force-pushed the skyzh/force-create-layers branch from c03b23a to 3c9ac19 Compare May 28, 2024 20:59
Copy link

github-actions bot commented May 28, 2024

3156 tests run: 3017 passed, 0 failed, 139 skipped (full report)


Flaky tests (1)

Postgres 15

  • test_statvfs_pressure_usage: debug

Code coverage* (full report)

  • functions: 31.4% (6503 of 20707 functions)
  • lines: 48.3% (50223 of 103881 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
b12c705 at 2024-05-31T15:53:31.681Z :recycle:

pageserver/src/tenant.rs Outdated Show resolved Hide resolved
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

I think this would move our tests into a direction which currently does what the previously invoked abstractions were doing, but also bypasses everything. I dislike this "alternative reality" and think this will incur great tech debt right away; instead we should investigate wrapping the TimelineWriter.

Discussion about above higher level test-api problem:
https://github.com/neondatabase/neon/pull/7899/files#r1620672536

@skyzh
Copy link
Member Author

skyzh commented May 30, 2024

I don't feel timeline writer is the right thing to wrap for this test API. Timeline writer represents a normal+legal write to the timeline. The layers written by the timeline writer API goes through L0->compaction->L1. However, in my case, I want to construct a timeline state that is exactly what I want to test, instead of using high-level APIs to do a series of operations that may or may not end up in the state I want to test. As you see, the test cases are much simpler than before, and I don't see it would be a tech debt if this is only used in test cases.

Anyways, I feel what we lack is to describe a timeline state and directly test it in the test cases. For example, if I want to write a test "ensure vectored get does not go pass image layer", I will need to create two layers: an image layer above a delta layer. Using the current API, it's hard to achieve. It's hard to guarantee that after a series of compaction and data writing, the end state of the timeline is what I want. The interface to directly place a layer into the layer map is simple and reliable to recreate this scenario.

pageserver/src/tenant.rs Outdated Show resolved Hide resolved
Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh force-pushed the skyzh/force-create-layers branch from 9a06567 to 6d753d8 Compare May 30, 2024 18:31
pageserver/src/tenant.rs Outdated Show resolved Hide resolved
pageserver/src/tenant.rs Outdated Show resolved Hide resolved
Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

I think this is looking much better. We could tune the declarativeness aspect with more builder-y structures and not using the layer creation but going through TimelineWriter, but I think this would already work for vectored layer tests just fine. Now I think it's much more clear that we aim to have only single path for "timeline growing" methods, or alternatively these should be used instead.

Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh merged commit 87afbf6 into main May 31, 2024
64 checks passed
@skyzh skyzh deleted the skyzh/force-create-layers branch May 31, 2024 16:00
a-masterov pushed a commit that referenced this pull request Jun 3, 2024
This pull request adds necessary interfaces to deterministically create
scenarios we want to test. Simplify some test cases to use this
interface to make it stable + reproducible.

Compaction test will be able to use this interface. Also the upcoming
delete tombstone tests will use this interface to make test
reproducible.

## Summary of changes

* `force_create_image_layer`
* `force_create_delta_layer`
* `force_advance_lsn`
* `create_test_timeline_with_states`
* `branch_timeline_test_with_states`

---------

Signed-off-by: Alex Chi Z <chi@neon.tech>
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.

2 participants