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

Tests + Fix for non-empty string storageID tierFS #8603

Closed
nadavsteindler opened this issue Feb 5, 2025 · 2 comments · Fixed by #8616
Closed

Tests + Fix for non-empty string storageID tierFS #8603

nadavsteindler opened this issue Feb 5, 2025 · 2 comments · Fixed by #8616
Assignees
Labels
bug Something isn't working msb tech-debt

Comments

@nadavsteindler
Copy link
Contributor

nadavsteindler commented Feb 5, 2025

Debt from the StorageID tierFS issue. As per Ariel's comment:
#8549 (review)

On the dev side, I am unable to find anywhere that tests a non-empty storage ID. This might be worrying? (This request is blocking: we should test non-empty storage IDs, or state on the description of this PR why the existing tests are sufficient, or keep the issue open until we do have a PR that tests it.)

Note the comment in the design: the StorageID will currently only be allowed to be empty/nil.

What levels should we test at?

  • TierFS- this is the lowest level change and interesting stuff happens at this layer. Add tests
  • committed manager- we will significantly change & test it as part of Range and Metarange per StorageID #8595
  • Catalog- didn't significantly change(yet)
@arielshaqed
Copy link
Contributor

I understand that this issue is to test TierFS with nondefault StorageIDs. This is really important! Please note that #8549 is actually broader than its title makes it sound; in addition to TierFS, it adds StorageIDs to at least:

  • Catalog
  • Committed data
    • including import
    • Commits and merges
  • Range and metarange managers and writers

All this plumbing has to work, so we will have to test it. By deciding to pass StorageID as a parameter rather than on a context or on objects, we pretty much committed to testing all those parameters.

@nadavsteindler nadavsteindler changed the title Tests for non-empty string storageID tierFS Tests + Fix for non-empty string storageID tierFS Feb 10, 2025
@nadavsteindler nadavsteindler added the bug Something isn't working label Feb 10, 2025
@nadavsteindler
Copy link
Contributor Author

I understand that this issue is to test TierFS with nondefault StorageIDs. This is really important! Please note that #8549 is actually broader than its title makes it sound; in addition to TierFS, it adds StorageIDs to at least:

  • Catalog

  • Committed data

    • including import
    • Commits and merges
  • Range and metarange managers and writers

All this plumbing has to work, so we will have to test it. By deciding to pass StorageID as a parameter rather than on a context or on objects, we pretty much committed to testing all those parameters.

Sorry I didn't write out all my thoughts. I want to also test CommitedManager and Catalog BUT Catalog didn't significantly change yet in that PR and CommittedData will change in the Ranges & Metaranges PR so I will write those tests there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working msb tech-debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants