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

add lotus-miner storage-deals list --format=json with transfers #7312

Merged
merged 5 commits into from
Sep 24, 2021

Conversation

nonsense
Copy link
Member

@nonsense nonsense commented Sep 10, 2021

This PR is updating the lotus-miner storage-deals list cmd to print out storage deals together with their respective transfers, for easier debugging - only when using --format=json. At the moment fetching storage deals and transfers is done via two separate commands and somewhat cumbersome to use given the tab-delimited tables.

--format=json prints out results in line-delimited JSON, which I think is much easier to read than the table approach we use in many commands, because:

  1. Field type is right next to every value (given that we generally have 100s-1000s of deals):
{"datetime":"2021-09-10T00:57:41+03:00","verified-deal":true,"proposal-cid":"bafyreif5yiywi6njej3nz2uwrj6yo2dnrc7mnokk3qhhfc6m46efpurmay","deal-id":0,"deal-status":"StorageDealTransferring","client":"f3vnq2cmwig3qjisnx5hobxvsd4drn4f54xfxnv4tciw6vnjdsf5xipgafreprh5riwmgtcirpcdmi3urbg36a","piece-size":"16GiB","price":"0 FIL","duration-epochs":1494720,"transfer-id":1631214107052231430,"transfer-status":"Ongoing","transferred-data":"0B"}
  1. Searching and formatting is very easy with grep and jq:
$ LOTUS_MARKETS_PATH=~/markets-repo-location ./lotus-miner storage-deals --format=json | grep 7vsgutuiem | jq
{
  "datetime": "2021-09-10T11:36:48+03:00",
  "verified-deal": true,
  "proposal-cid": "bafyreie2e66s3ztkdrfoqxm7hrch5d75i3nbrtp6jnsdcyrr7vsgutuiem",
  "deal-id": 0,
  "deal-status": "StorageDealTransferring",
  "client": "f3vnq2cmwig3qjisnx5hobxvsd4drn4f54xfxnv4tciw6vnjdsf5xipgafreprh5riwmgtcirpcdmi3urbg36a",
  "piece-size": "8GiB",
  "price": "0 FIL",
  "duration-epochs": 1494720,
  "transfer-id": 1631214107052231700,
  "transfer-status": "Ongoing",
  "transferred-data": "0B"
}

@dirkmc
Copy link
Contributor

dirkmc commented Sep 10, 2021

Thanks for putting this together 👍

I agree the JSON output is clearer and easier to parse.

Would it make sense to add the output format and a flag for whether to include deal transfers onto the existing list command?
Something like
./lotus-miner storage-deals list --format=json --with-transfers

@nonsense
Copy link
Member Author

Would it make sense to add the output format and a flag for whether to include deal transfers onto the existing list command? Something like
./lotus-miner storage-deals list --format=json --with-transfers

I actually started with that, and it gets messy very quickly to support --with-transfers in the --format=table so I decided to just go with a separate command and keep things simpler. I don't have hard feelings about that though, so I could bundle it together with list, but maybe add --with-transfers only for --format=json? WDYT?

@nonsense nonsense changed the title add lotus-miner storage-deals list-with-transfers add lotus-miner storage-deals list --with-transfers and --format=json Sep 10, 2021
@nonsense nonsense changed the title add lotus-miner storage-deals list --with-transfers and --format=json add lotus-miner storage-deals list --format=json with transfers Sep 10, 2021
@nonsense nonsense force-pushed the nonsense/crossref-datatransfer-storagedeal branch from b486eb1 to fe602ab Compare September 10, 2021 13:06
@nonsense
Copy link
Member Author

It is quite hard to support --watch also for data transfers, so I opted for:

  1. Not adding --with-transfers flag at all - always try to load transfers when using --format=json
  2. Leave --format=table as is, as we might want to remove --watch altogether.

@nonsense nonsense marked this pull request as ready for review September 10, 2021 13:18
@nonsense nonsense requested a review from a team as a code owner September 10, 2021 13:18
@nonsense nonsense requested a review from dirkmc September 10, 2021 13:19
},
}

func listDealsWithTable(cctx *cli.Context) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

For easier review - this is not modified.

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #7312 (3e10f2e) into master (0a04302) will decrease coverage by 0.07%.
The diff coverage is 2.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7312      +/-   ##
==========================================
- Coverage   39.14%   39.06%   -0.08%     
==========================================
  Files         614      614              
  Lines       64997    65056      +59     
==========================================
- Hits        25440    25412      -28     
- Misses      35150    35244      +94     
+ Partials     4407     4400       -7     
Impacted Files Coverage Δ
cmd/lotus-miner/market.go 4.78% <2.22%> (-4.70%) ⬇️
chain/actors/builtin/miner/diff.go 48.52% <0.00%> (-10.30%) ⬇️
miner/miner.go 52.64% <0.00%> (-3.98%) ⬇️
chain/sub/incoming.go 54.43% <0.00%> (-3.38%) ⬇️
storage/wdpost_sched.go 75.24% <0.00%> (-1.99%) ⬇️
chain/exchange/client.go 52.96% <0.00%> (-1.70%) ⬇️
extern/sector-storage/sched_worker.go 77.25% <0.00%> (-1.45%) ⬇️
chain/gen/gen.go 66.06% <0.00%> (-1.22%) ⬇️
chain/sync_manager.go 66.77% <0.00%> (-0.63%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a04302...3e10f2e. Read the comment docs.

Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nonsense nonsense requested a review from raulk September 14, 2021 09:50
@elliot-u410
Copy link

In #7235 I used a --json flag. Should there be a standard approach to this? I'd like to point out that AFAIK the table output doesn't actually use TSV. The tabs get converted to spaces. This makes parsing it harder than it should be IMO.

@nonsense
Copy link
Member Author

In #7235 I used a --json flag. Should there be a standard approach to this? I'd like to point out that AFAIK the table output doesn't actually use TSV. The tabs get converted to spaces. This makes parsing it harder than it should be IMO.

Yes, ideally we should be consistent across various command line tools. Having said that I don't have any preference over --format=json or just --json and --format=table and --table, but probably better to be explicit.

For now I will leave the other reviewers to chime in.

@magik6k magik6k merged commit 0e7e665 into master Sep 24, 2021
@magik6k magik6k deleted the nonsense/crossref-datatransfer-storagedeal branch September 24, 2021 09:58
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