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

fix(sui-indexer): allow setting --gc-checkpoint-files=false #20783

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

unmaykr-aftermath
Copy link
Contributor

@unmaykr-aftermath unmaykr-aftermath commented Jan 5, 2025

Description

Previously I was unable to turn off checkpoint file garbage collection,
since the default value was true, the --gc-checkpoint-files flag
didn't expect any values and the default clap action was to set the
value to true when the flag was present. I've tried all of:

  • --gc-checkpoint-files 0
  • --gc-checkpoint-files false
  • --gc-checkpoint-files=0
  • --gc-checkpoint-files=false

This changes that so that the false value is accepted by the argument, i.e., you can set --gc-checkpoint-files=false. Default behavior when the argument is missing remains the same as before.

It is based on this comment

Test plan

How did you test the new or updated feature?


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):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

Previously I was unable to turn off checkpoint file garbage collection,
since the default value was `true`, the `--gc-checkpoint-files` flag
didn't expect any values and the default `clap` action was to set the
value to `true` when the flag was present. I've tried all of:

- `--gc-checkpoint-files 0`
- `--gc-checkpoint-files false`
- `--gc-checkpoint-files=0`
- `--gc-checkpoint-files=false`

This changes that so that the `false` value is accepted by the argument.
It is based on this [comment](clap-rs/clap#1649 (comment))
Copy link

vercel bot commented Jan 5, 2025

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

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2025 6:14pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2025 6:14pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Jan 5, 2025 6:14pm

@amnn amnn requested review from lxfind, amnn, gegaowp, emmazzz and wlmyng January 5, 2025 18:18
@unmaykr-aftermath unmaykr-aftermath temporarily deployed to sui-typescript-aws-kms-test-env January 5, 2025 19:00 — with GitHub Actions Inactive
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.

Interesting, thanks for finding this. lgtm since we're keeping the existing behavior when gc-checkpoint-files is not set (although perhaps this was not the intention either, looking at the doc comment)

@wlmyng wlmyng merged commit 0a03627 into MystenLabs:main Jan 8, 2025
46 checks passed
This was referenced Feb 10, 2025
@gegaowp
Copy link
Contributor

gegaowp commented Feb 10, 2025

this caused a CLI issue reported at #21037 (comment)
and I put up a fix PR #21158

gegaowp added a commit that referenced this pull request Feb 10, 2025
## Description 

an issue reported by community 

#21037 (comment)

introduced in 
#20783

the error was 
```
Argument `gc_checkpoint_files`'s selected action SetTrue contradicts `takes_value`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

## Test plan 

```
DB_POOL_SIZE=10 cargo run --bin sui-indexer -- --db-url "postgres://postgres:postgrespw@localhost:5432/gegao" indexer --remote-store-url https://checkpoints.mainnet.sui.io 
DB_POOL_SIZE=10 cargo run --bin sui-indexer -- --db-url "postgres://postgres:postgrespw@localhost:5432/gegao" indexer --remote-store-url https://checkpoints.mainnet.sui.io --gc-checkpoint-files
DB_POOL_SIZE=10 cargo run --bin sui-indexer -- --db-url "postgres://postgres:postgrespw@localhost:5432/gegao" indexer --remote-store-url https://checkpoints.mainnet.sui.io --gc-checkpoint-files=true
DB_POOL_SIZE=10 cargo run --bin sui-indexer -- --db-url "postgres://postgres:postgrespw@localhost:5432/gegao" indexer --remote-store-url https://checkpoints.mainnet.sui.io --gc-checkpoint-files=false
DB_POOL_SIZE=10 cargo run --bin sui-indexer -- --db-url "postgres://postgres:postgrespw@localhost:5432/gegao" indexer --remote-store-url https://checkpoints.mainnet.sui.io --gc-checkpoint-files true
DB_POOL_SIZE=10 cargo run --bin sui-indexer -- --db-url "postgres://postgres:postgrespw@localhost:5432/gegao" indexer --remote-store-url https://checkpoints.mainnet.sui.io --gc-checkpoint-files false
```


---

## 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): 
- [ ] gRPC:
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
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.

3 participants