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

Pintracker/local operation logic revamp #308

Closed
hsanjuan opened this issue Jan 29, 2018 · 1 comment
Closed

Pintracker/local operation logic revamp #308

hsanjuan opened this issue Jan 29, 2018 · 1 comment
Assignees
Labels
exp/intermediate Prior experience is likely helpful exp/wizard Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked

Comments

@hsanjuan
Copy link
Collaborator

A post to ipfs will hang and doesn't time out unless ipfs itself does.

We need:

  • Timeout control for the POST operations to ipfs
  • Bonus: configurable in the config
  • Bonus: every ongoing operation is tracked with a context and cancellable at any moment
  • Bonus: ipfsconn exposes ongoing operations and a method to cancel them
  • Bonus: the pintracker (maptracker) can make use of those when it is pinning an item and receives an unpin request (cancel any pinning operation for that item and unpin)
  • Bonus: pintracker removes any related pin operations from the pin queue
@hsanjuan hsanjuan added kind/enhancement A net-new feature or improvement to an existing feature help wanted Seeking public contribution on this issue exp/wizard Extensive knowledge (implications, ramifications) required exp/intermediate Prior experience is likely helpful status/ready Ready to be worked P2 Medium: Good to have, but can wait until someone steps up labels Jan 29, 2018
@hsanjuan hsanjuan changed the title ipfsconn component should allow specifying ipfs-operations timeout Pintracker/local operation logic revamp Feb 22, 2018
@hsanjuan
Copy link
Collaborator Author

I am going to re-use this issue to describe what we need to do in order to improve local operation handling (that is, a pin/unpin/sync/recover requests that try to match the ipfs state to the shared state).

This is logic taken care of by the mappintracker component which talks to the httpipfsconn component as needed.

There are a bunch of problems:

  • When something needs to be pinned if goes into pinning state, but we have no way to know if it's just queuing because some other thing is pinning or it's actually being pinned (waiting for ipfs pin request to succeed)
  • Once something is queued to pin there is no way to remove it from the queue
  • Once something is queued to pin there is no way to know how far behind in the queue it sits
  • sync() makes pin status show a timeout error when something has not been pinned in less than pinning_timeout since it was added to the queue, but it is not removed from the queue if it was waiting. Things should not timeout if it was just queued and the "stuck" pin is something else. Since pins are still queued, error states may well turn to pinned later if the queue is processed.
  • Due to the above recover() resubmits items to the queue, even if they are still queued.
  • RecoverAll() should not return REMOTE items. It should really only return pins which it has recovered or tried to.
  • We have no way of canceling an ongoing pin from the pin tracker (see first comment in this issue)
  • If an Unpin() request is received, and something is queued or waiting to be pinned, then it's for nothing.

The pin tracker works OK in an scenario with small amount of pin requests that can be processed in time, but not with large sets of pins injected to cluster. The pin tracker implementation is relatively old and naive and needs to step up to current needs.

hsanjuan added a commit that referenced this issue Mar 7, 2018
This fixes #326. It adds a new `pin_method` configuration option to the
`ipfshttp` component allows to configure it to perform `refs -r <cid>` before
the `pin/add` call. By fetching content before pinning, we don't have
a global lock in place, and we can have several pin-requests to
ipfs in parallel.

It also adds a `concurrent_pins` option to the pin tracker, which
launches more pin workers so it can potentially trigger more pins at
the same time. This is a minimal intervention in the pintracker as #308
is still pending.

Documentation for the configuration file has been updated.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
hsanjuan added a commit that referenced this issue Mar 7, 2018
This fixes #326. It adds a new `pin_method` configuration option to the
`ipfshttp` component allows to configure it to perform `refs -r <cid>` before
the `pin/add` call. By fetching content before pinning, we don't have
a global lock in place, and we can have several pin-requests to
ipfs in parallel.

It also adds a `concurrent_pins` option to the pin tracker, which
launches more pin workers so it can potentially trigger more pins at
the same time. This is a minimal intervention in the pintracker as #308
is still pending.

Documentation for the configuration file has been updated.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@hsanjuan hsanjuan added P1 High: Likely tackled by core team if no one steps up and removed P2 Medium: Good to have, but can wait until someone steps up help wanted Seeking public contribution on this issue labels Mar 9, 2018
hsanjuan added a commit that referenced this issue Mar 9, 2018
This fixes #326. It adds a new `pin_method` configuration option to the
`ipfshttp` component allows to configure it to perform `refs -r <cid>` before
the `pin/add` call. By fetching content before pinning, we don't have
a global lock in place, and we can have several pin-requests to
ipfs in parallel.

It also adds a `concurrent_pins` option to the pin tracker, which
launches more pin workers so it can potentially trigger more pins at
the same time. This is a minimal intervention in the pintracker as #308
is still pending.

Documentation for the configuration file has been updated.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@lanzafame lanzafame self-assigned this Apr 3, 2018
@lanzafame lanzafame added the status/in-progress In progress label Apr 4, 2018
@hsanjuan hsanjuan mentioned this issue May 7, 2018
@hsanjuan hsanjuan closed this as completed Jun 1, 2018
@ghost ghost removed the status/in-progress In progress label Jun 1, 2018
@hsanjuan hsanjuan mentioned this issue Aug 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/intermediate Prior experience is likely helpful exp/wizard Extensive knowledge (implications, ramifications) required kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

2 participants