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

Run SM tests agains SSTables with UUID ID #4182

Open
Michal-Leszczynski opened this issue Dec 18, 2024 · 2 comments · May be fixed by #4197
Open

Run SM tests agains SSTables with UUID ID #4182

Michal-Leszczynski opened this issue Dec 18, 2024 · 2 comments · May be fixed by #4197
Assignees

Comments

@Michal-Leszczynski
Copy link
Collaborator

SSTables with UUID ID were introduced to Scylla a long time ago, but we disable them in SM tests with the following scylla.yaml configuration:

uuid_sstable_identifiers_enabled: false

We do it because we wanted to test the backup deduplication mechanism and it required some SSTables with integer based IDs.

It would be nice if we could just test the UUID IDs, as this is the default and most common case, but unfortunately for us, many users can still have both integer based and UUID SSTables in the same cluster (only new SSTables are written with UUIDs while some old ones could never been re-written and still have integer based ID).

For now I see 3 ways to tackle this problem:

  • introduce another test env param (e.g. UUIDSSTABLES) which could control uuid_sstable_identifiers_enabled.
    This is quick and easy, but it wouldn't allow for testing mixed ID scenarios.
  • always enable UUID SSTables, but manually rename some UUID SSTables to integer based ones after taking the snapshot and before deduplicating the files in tests which needs to verify integer based SSTable deduplication.
    This is probably the most correct approach, but it would require the most effort as well.
  • test deduplication on some pre-created snapshot dirs with mixed SSTable IDs.
    There is a question of how easy would it be to integrate pre-created SSTables with our test scenarios which usually create a real backup from a real cluster.
Michal-Leszczynski added a commit that referenced this issue Jan 10, 2025
UUID SSTables are the default in newer Scylla version,
so we should focus on testing them and mock integer
based SSTables when needed.

Ref #4182
@Michal-Leszczynski Michal-Leszczynski self-assigned this Jan 10, 2025
@Michal-Leszczynski
Copy link
Collaborator Author

Interestingly enough, if we simply don't force the creation of integer based SSTables, the tests are passing!
But they are failing, when we combine this with the native backup Scylla API...

The tests are passing because in the deduplication stage, we compare UUID SSTables by both name and size.
The size comparison causes SSTables not to be deduplicated, so then the Rclone backup API creates versioned files.

The tests are failing, because the code I wrote for checking if the backup would create versioned files compares UUID SSTables only by name.

Anyway, just enabling the UUID identifiers and never testing against integer identifiers does not give us full coverage, so I will go with the second approach of manually renaming SSTables in the snapshot dir.

@Michal-Leszczynski
Copy link
Collaborator Author

A side note is that we should align the way in which we compare UUID SSTables.
They should be compared just by the name - we don't want to carry the assumption that versioned and UUID SSTables could exist.

Michal-Leszczynski added a commit that referenced this issue Jan 15, 2025
This way we are able to cover mixed scenarios (integer + UUID SSTables),
which was missing from our test coverage. It also allows us to move
to testing the default UUID SSTables, which should be our priority.

Fixes #4182
Michal-Leszczynski added a commit that referenced this issue Jan 16, 2025
This way we are able to cover mixed scenarios (integer + UUID SSTables),
which was missing from our test coverage. It also allows us to move
to testing the default UUID SSTables, which should be our priority.

Fixes #4182
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 a pull request may close this issue.

1 participant