feat(crates/sui): add --with-data-ingestion to 'sui start'

[unmaykr-aftermath]

Description

Adds the --data-ingestion-dir option to sui start that allows users to set a custom data ingestion directory where checkpoint files will be written to.

This is useful, among other things, for testing a custom local reader indexing setup with low overhead.

This is not a breaking change, as sui start --with-indexer works as usual by creating a temporary directory for data ingestion, but now one can:

• Use sui start --data-ingestion-dir <path> --with-indexer to also have checkpoints written to <path>. May be useful for inspecting the data
• Use sui start --data-ingestion-dir <path> to only have checkpoints written to <path>, but not start an indexer service

P.s.: every time I ran sui start --with-data-ingestion=<path> (no other arguments), the network started producing checkpoints from 0. Is this intended or was the network supposed to pick up from where it left off? Perhaps it's the full node syncing.

Test plan

I tested it locally to verify that the checkpoints are indeed written to the configured directory. See the screenshot below. sui start --with-indexer still runs without problems and I verified that checkpoints were committed to the DB.

I set export SUI_CONFIG_DIR=~/.sui/test for the above.

---

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: Added the --data-ingestion-dir option to sui start to set a custom directory where to write the checkpoint files.
• Rust SDK:
• REST API:

[vercel]

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	Nov 29, 2024 8:57pm

3 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2024 8:57pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2024 8:57pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2024 8:57pm

[amnn]

Thanks @unmaykr-aftermath ! The change seems reasonable. I've left some suggestions for how to preserve the backwards compatibility.

I'm not very familiar with how the local file based data ingestion is supposed to work, so cc @bmwill and @phoenix-o to judge whether the behaviour of always starting from the genesis checkpoint is expected.

[amnn]

We can do something like this instead to retain the backwards compatibility:

Suggested change

	// the indexer requires to set the fullnode's data ingestion directory
	// note that this overrides the default configuration that is set when running the genesis
	// command, which sets data_ingestion_dir to None.
	ensure!(
	with_data_ingestion.is_some(),
	"Cannot start the indexer without --with-data-ingestion."
	);
	}
	if with_indexer.is_some() && with_data_ingestion.is_none() {
	with_data_ingestion = Some(tempdir()?.into_path())
	}

[amnn]

Would need to change this comment along with the suggestion above.

[amnn]

spurious change?

[unmaykr-aftermath]

Ah, I didn't notice it. I had the taplo LSP running

[amnn]

I think with the suggested change to how we handle the default/None case, we can get rid of all changes here.

[stefan-mysten]

Thanks @unmaykr-aftermath. Just a couple of nits as I was thinking about this, mostly food for thought.
The way I thought of with- flags is that they correspond to specific services. In that sense, I'd think more of the with-data-ingestion to be more like set-ingestion-dir (or data-ingestion-dir or some variation), as this does not start a specific service but instead sets up a service's config in some specific way similarly to what we do with --pg-port` and other args. What do you think?

Finally, I second the suggestion to not break existing way of starting the local network with sui start --with-indexer as it will break a lot of people's configs (and our own CI - which can be fixed, but it's always annoying as people will be forced to learn what's the new correct way).

[stefan-mysten]

Ah, and we should update the release notes with a description of what changed in the CLI section. I'll leave it to you and will double check before merging. Thanks again! @unmaykr-aftermath

[unmaykr-aftermath]

Thanks @unmaykr-aftermath. Just a couple of nits as I was thinking about this, mostly food for thought. The way I thought of with- flags is that they correspond to specific services. In that sense, I'd think more of the with-data-ingestion to be more like set-ingestion-dir (or data-ingestion-dir or some variation), as this does not start a specific service but instead sets up a service's config in some specific way similarly to what we do with --pg-port` and other args. What do you think?

I agree. I'll go with --data-ingestion-dir. And I'll also see where to write the release notes. Thanks for the feedback

[stefan-mysten]

And I'll also see where to write the release notes

This is right in the PR after Test Plan - the CLI checkbox is already selected, so we just need to add some description with the change.

[unmaykr-aftermath]

@amnn @stefan-mysten I updated the PR's desc. and made the suggested code changes

[stefan-mysten]

Thanks for the quick changes @unmaykr-aftermath. I'll give it a spin locally and then we can merge this!

[stefan-mysten]

@unmaykr-aftermath looks good. I will fix the failing CI issues (tests + fmt) and then merge. Thanks for this.