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

Feat/subselector retrieval #12

Merged
merged 3 commits into from
Oct 4, 2021
Merged

Feat/subselector retrieval #12

merged 3 commits into from
Oct 4, 2021

Conversation

ribasushi
Copy link
Contributor

This is an amalgam of filecoin-project/lotus#6393 and the abandoned application-research/estuary#5, providing a fully featured selector retrieval to filc

Given a random cid status like this, one should be able to:

filc retrieve -m {{chosenminer}} --datamodel-path-selector Links/217/Hash/Links/30/Hash/Links/0/Hash bafybeihco4vemvoyuca7vqomert5fuvf6deksmakftu654mipagdkehzyu

Copy link
Contributor

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but i dont know the selectors stuff very well. Did you manage to test this against a miner @ribasushi ?

@ribasushi
Copy link
Contributor Author

ribasushi commented Sep 11, 2021

@whyrusleeping unfortunately not end-to-end: I just can't get filc to work in general 98% of the time 😿

The code is solid because I have this implemented: https://github.com/filecoin-project/lotus/blob/0444435589d0c6817072278e222346829ed637a4/itests/deals_partial_retrieval_test.go#L25-L113
This PR is a c/p of that more or less.

@ribasushi
Copy link
Contributor Author

@elijaharita think you can test the steps described here? perhaps you'll have more luck...
#12 (comment)

@ribasushi ribasushi force-pushed the feat/subselector_retrieval branch from 41e426e to 481c888 Compare September 12, 2021 22:48
@ribasushi
Copy link
Contributor Author

Foce-pushed to subsume what is already merged: the diff is now even smaller
@elijaharita I am really uncomfortable merging this until you manage to test it though ☹️

Copy link
Contributor

@elijaharita elijaharita left a comment

Choose a reason for hiding this comment

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

Tested, seems to be working correctly! Still experiencing retrieval issues as usual, but that is not under our control. Looks good :)

@ribasushi
Copy link
Contributor Author

@rvagg you raised a number of concerns over at filecoin-project/lotus#6393 (comment)

Do you have objections to merging things as-is in this repo? If yes - could you outline which points you see as "must fix" so we can move things along at least here?

@rvagg
Copy link
Collaborator

rvagg commented Sep 14, 2021

No objections from me, just a couple of notes that may be taken on board or discarded:

  • the comment for what the selector is for could be clarified to say that it's a path-based selector for the sub-root, it's not a general selector for arbitrary pieces of the dag, just a path to a specific block to start the dag from
  • this same problem of the selector being able to reach deeply into arbitrary nodes within a block, it's not necessarily a problem but the looseness around being able to get exactly the same results from Links/5/Hash Links/5/Hash/Data, Links/5/Hash/Links/100, Links/5/Hash/Links/0/Name, etc., as I noted over in Feat/datamodel selector retrieval filecoin-project/lotus#6393, might create a squishy API surface area that could lead to future problems. I don't know (yet) how to easily tell whether you've walked too far into a block but ISTM that the ideal might be to reject selectors that don't stop at a block boundary. For now though maybe it doesn't matter too much.

I'll try and get this path-to-selector stuff you did into go-ipld-prime and maybe we can come up with mechanisms to handle the same issue here and in lotus when that's done. I think Eric's OK with it being in there along with other utilities to do similar classes of things.

@ribasushi
Copy link
Contributor Author

@rvagg your comments (crudely) addressed here in 4940219 and in lotus ( the latter is more important as it has end-to-end tests )

I am going to close this out at least in this repo, we can tune in subsequent PRs.

@ribasushi ribasushi merged commit 7664728 into master Oct 4, 2021
@ribasushi ribasushi deleted the feat/subselector_retrieval branch October 4, 2021 21:41
@rvagg
Copy link
Collaborator

rvagg commented Oct 5, 2021

👍 if that works, then that's a tidy neat solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants