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

indexer: exclude deleted/wrapped objects from snapshot #19455

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

gegaowp
Copy link
Contributor

@gegaowp gegaowp commented Sep 19, 2024

Description

https://mysten-labs.slack.com/archives/C0578KFD9D2/p1726765436840499
this will cut storage further down

Test plan

objects snapshot ingestion test


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • REST API:

Copy link

vercel bot commented Sep 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 9, 2024 8:17pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 8:17pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 8:17pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Oct 9, 2024 8:17pm

Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

this might break a bunch of graphql tests because we expect partial data (ObjectRef) to be available, and we map that to ObjectStatus::WrappedOrDeleted. Now that we are excluding wrapped/deleted objects from the snapshot, these will yield empty results

@gegaowp
Copy link
Contributor Author

gegaowp commented Sep 19, 2024

this might break a bunch of graphql tests because we expect partial data (ObjectRef) to be available, and we map that to ObjectStatus::WrappedOrDeleted. Now that we are excluding wrapped/deleted objects from the snapshot, these will yield empty results

@wlmyng thx for the quick review! do I just need to look out and try to fix those tests, or there are real dependencies that we should hold on this change?

@amnn
Copy link
Member

amnn commented Sep 20, 2024

The dependency is real today, so the first step would be to remove the references to this kind of information from the GraphQL side.

@gegaowp gegaowp changed the title indexer: exclude deleted / wrapped objects from snapshot indexer: exclude deleted/wrapped objects from snapshot & checkpoint batch fix Sep 20, 2024
@gegaowp gegaowp requested a review from lxfind September 20, 2024 16:28
@gegaowp
Copy link
Contributor Author

gegaowp commented Sep 20, 2024

@amnn sounds good, I also found a bug while writing this pr, I will split them then. regarding the references removal, do we have linear issue tracking, can we make one if not?

@amnn
Copy link
Member

amnn commented Sep 23, 2024

do we have linear issue tracking, can we make one if not?

I don't think so -- feel free to add one.

@gegaowp gegaowp changed the title indexer: exclude deleted/wrapped objects from snapshot & checkpoint batch fix indexer: exclude deleted/wrapped objects from snapshot Oct 9, 2024
@gegaowp
Copy link
Contributor Author

gegaowp commented Oct 9, 2024

I am merging the other pre-requisite pr #19737 and this pr is now ready for another review @bmwill @wlmyng @amnn thanks!

@gegaowp gegaowp enabled auto-merge (squash) October 9, 2024 20:17
@gegaowp gegaowp merged commit d09e3ed into MystenLabs:main Oct 9, 2024
45 checks passed
@gegaowp gegaowp deleted the exclude-deleted-obj branch October 9, 2024 21:21
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.

4 participants