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

API: Allow user to set peer allocations for pinning #647

Merged
merged 2 commits into from Feb 25, 2019
Merged

Conversation

kishansagathiya
Copy link
Contributor

@kishansagathiya kishansagathiya commented Jan 16, 2019

Added support for allocations query value in POST /pins/<cid>
endpoint
allocations will be a comma-separated list of peer ids to which we want
to pin.
allocations will be prioritized over other query values of similar
effect such as replication factor, replication factor min and
replication factor max

  • support this in command as well
  • tests

Fixes #646

@ghost ghost assigned kishansagathiya Jan 16, 2019
@ghost ghost added the status/in-progress In progress label Jan 16, 2019
@coveralls
Copy link

coveralls commented Jan 16, 2019

Coverage Status

Coverage decreased (-0.6%) to 65.049% when pulling 336e639 on issue_646 into a244af9 on master.

Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

Will need client and api tests (I don't remember if ipfscluster_test.go does something with priority allocations, but I would expect so since I think they are used in sharding.

api/rest/client/methods.go Outdated Show resolved Hide resolved
api/rest/restapi.go Outdated Show resolved Hide resolved
cluster.go Outdated Show resolved Hide resolved
`,
ArgsUsage: "<CID>",
Flags: []cli.Flag{
cli.StringFlag{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move flag after replication factor.

cmd/ipfs-cluster-ctl/main.go Outdated Show resolved Hide resolved
cmd/ipfs-cluster-ctl/main.go Show resolved Hide resolved
cmd/ipfs-cluster-ctl/main.go Outdated Show resolved Hide resolved
@hsanjuan
Copy link
Collaborator

Per the PinPath PR, I'm thinking how to best integrate Allocations as part of PinOptions.

One way would be to move Allocations to PinOptions. Since it's an array of peer IDs, we will want to have PinOptionsSerial in which it becomes an array of strings. As with other types, we will need methods to convert between them. This will cascade to api.PinSerial (which will need to carry PinOptionsSerial instead of PinOptions), and eventually will need a PinPathSerial in the same way.

Another way would be to add a UserAllocations []peer.ID field. peer.ID are actually a string (a byte slice casted into a string actually). This is perfectly serializable, however we cannot spit it out of the API without calling something like PeersToStrings() first. We could however, have PinOptions implement json.Marshaler and json.Unmarshaler interface and this will make sure it spitted out correctly out of the API. This is similar to what we started doing with the Metric type.

Therefore, for the moment, I'm leaning towards the latter option. Also, keeping separate user-selected allocations and system-given ones seems useful as it allows us to compare if the requested ones were fulfilled.

@hsanjuan hsanjuan changed the title Pin file only to certain nodes API: Allow user to set peer allocations for pinning Jan 23, 2019
@hsanjuan
Copy link
Collaborator

I just checked and peer.ID implements JSON marshable interfaces, so it may work "as is", leaving UserAllocations as []peer.ID, but needs testing.

Copy link
Contributor

@lanzafame lanzafame left a comment

Choose a reason for hiding this comment

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

@hsanjuan has already said all that needs to be said at this point.

@hsanjuan hsanjuan mentioned this pull request Feb 20, 2019
@kishansagathiya
Copy link
Contributor Author

kishansagathiya commented Feb 22, 2019

@hsanjuan
In situations when replication factor and allocations don't match, say replication factor is 2, but allocations have more than 2 peers, which one should take priority? Allocations? or error out?

`allocations` will be a comma-separated list of peer IDs on which we
want to pin. Peers in allocations are prioritized over automatically-determined
ones, but replication factors would stil be respected.
@hsanjuan
Copy link
Collaborator

@hsanjuan
In situations when replication factor and allocations don't match, say replication factor is 2, but allocations have more than 2 peers, which one should take priority? Allocations? or error out?

The allocator code will choose enough allocations to satisfy the replication factor. Replication factor has priority.

Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@hsanjuan
Copy link
Collaborator

Let's wait for #681 to be merged.

@hsanjuan hsanjuan added status/blocked Unable to be worked further until needs are met RFM Ready for Merge and removed status/in-progress In progress labels Feb 24, 2019
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@ghost ghost assigned hsanjuan Feb 25, 2019
@ghost ghost added the status/in-progress In progress label Feb 25, 2019
@hsanjuan hsanjuan merged commit 13d56b4 into master Feb 25, 2019
@ghost ghost removed the status/in-progress In progress label Feb 25, 2019
@hsanjuan hsanjuan deleted the issue_646 branch February 25, 2019 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM Ready for Merge status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants