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

implements a stateless pintracker #460

Merged
merged 9 commits into from Aug 15, 2018

Conversation

Projects
None yet
3 participants
@lanzafame
Copy link
Collaborator

lanzafame commented Jun 8, 2018

Also updates to the optracker to make retrieving information easier.

This PR replaces #422.


This change is Reviewable

@lanzafame lanzafame requested a review from hsanjuan Jun 8, 2018

@wafflebot wafflebot bot added the in progress label Jun 8, 2018

@lanzafame lanzafame force-pushed the statelesstracker branch from ca2f0e5 to b65e4d7 Jun 11, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 11, 2018

Coverage Status

Coverage decreased (-1.7%) to 65.075% when pulling 949abb2 on statelesstracker into 5057947 on master.

@lanzafame lanzafame force-pushed the statelesstracker branch from b65e4d7 to 481bf2f Jun 11, 2018

@hsanjuan
Copy link
Collaborator

hsanjuan left a comment

I will re-visit this, particularly tests etc. Just some comments for the moment.

flag.Parse()

rand.Seed(time.Now().UnixNano())

for f := range LoggingFacilities {
SetFacilityLogLevel(f, logLevel)
if len(customLogLvlFacilities) < 0 {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 12, 2018

Collaborator

<0 is never?

This comment has been minimized.

@lanzafame

lanzafame Jun 13, 2018

Author Collaborator

Hmm, yeah, my bad.

}

for f := range LoggingFacilitiesExtra {
SetFacilityLogLevel(f, logLevel)
for _, f := range customLogLvlFacilities {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 12, 2018

Collaborator

I think I understand the purpose but how this works in the end is weird because the normal logging facilities will stay with default level (error i think). anyway, if it works for you it's fine

This comment has been minimized.

@lanzafame

lanzafame Jun 13, 2018

Author Collaborator

Yeah, it is so I could set just pintracker to debug and cut all the noise.

&gpin,
)
if err != nil {
if _, ok := err.(*api.CidNotInGlobalStateError); ok {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 12, 2018

Collaborator

As a note, the fact that this works is because of the local-rpc-shortcut part of gorpc. I think the right way to fix this is on gorpc, declaring custom error types there are having a function like gorpc.IsRPCError(err) to identify them.

Mind going that way instead?

This comment has been minimized.

@lanzafame

lanzafame Jun 13, 2018

Author Collaborator

Yeah, that sounds like a really good solution. 👍

}

func (spt *Tracker) removeError(c *cid.Cid) {
spt.optracker.Clean(spt.optracker.GetOp(c))

This comment has been minimized.

@hsanjuan

hsanjuan Jun 12, 2018

Collaborator

Can you add a cleanError(cid) to the optracker which gets an op and cleans it only if it's in error?

And then remove GetOp(c) from it. I don't like that you can get operations from it that you did not have beforehand (or are new).

@lanzafame

This comment has been minimized.

Copy link
Collaborator Author

lanzafame commented Jun 25, 2018

@hsanjuan I have updated this PR to use the new gorpc method.

@lanzafame lanzafame referenced this pull request Jun 25, 2018

Closed

Status filters for `ipfs-cluster-ctl status` #445

2 of 4 tasks complete
return false
}

for _, p := range pin.Allocations {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

We have a containsPeer() function in util.go. Perhaps we should promote it to api/util.go and re-use it here.

// CidNotInGlobalStateError allows for the differentiation of network
// and rpc errors from an error indicating that the Pin isn't in the
// global state.
type CidNotInGlobalStateError struct {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

This can go away with changes to the gorpc right?

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

cluster.go Outdated
@@ -888,7 +888,7 @@ func (c *Cluster) Pins() []api.Pin {
func (c *Cluster) PinGet(h *cid.Cid) (api.Pin, error) {
pin, ok := c.getCurrentPin(h)
if !ok {
return pin, errors.New("cid is not part of the global state")
return pin, &api.CidNotInGlobalStateError{Cid: h}

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

same, can be reverted

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

@@ -323,7 +327,7 @@ func TestClusterRecoverAllLocal(t *testing.T) {
defer cl.Shutdown()

c, _ := cid.Decode(test.TestCid1)
err := cl.Pin(api.PinCid(c))
err := cl.Pin(api.Pin{Cid: c, ReplicationFactorMax: -1})

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

When ReplicationFactorMax is 0 (as it was), it should take the default from the configuration (-1) and set accordingly somewhere in the code. If something failed because this wasn't set to -1, then the problem wasn't here... I'd rather have this reverted to the previous form.

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

Hmm, yeah I think I was making sure whilst debugging something. Removed.

@@ -375,3 +375,8 @@ func (mpt *MapPinTracker) SetClient(c *rpc.Client) {
mpt.rpcClient = c
mpt.rpcReady <- struct{}{}
}

// OpContext exports the internal optracker's OpContext method.

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

Maybe we should mention this mostly an accessory for testing. This method should not be used in the libs.

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

I had the comment on the stateless tracker one, 🤦‍♂️

}
var cspis []api.Pin
for _, p := range csps {
cspis = append(cspis, p.ToPin())

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

hmm, the fact that the whole state needs to be serialized, only to re-convert it to the original form supports my idea that we could pass the state component directly to this pintracker during initialization and work directly with it.

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

maybe 🙃

pininfos[p.Cid.String()] = api.PinInfo{
Cid: p.Cid,
Peer: spt.peerID,
Status: api.TrackerStatusRemote,

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

+TS

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

See ipfsStatus comment re: retrieval vs update

for _, p := range cspis {
if p.IsRemotePin(spt.peerID) && incRemote {
// add pin to pininfos with a status of remote
pininfos[p.Cid.String()] = api.PinInfo{

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

.String() is a bit expensive operation for Cids. Might as well store it in a variable and re-use.

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

return spt.optracker.Filter(optracker.PhaseError)
}

func (spt *Tracker) removeError(c *cid.Cid) {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

this is not used. Also, CleanError should probably be used (atomic removal of error), but it can be called directly rather than having a wrapping method for the single call.

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

}

for _, p := range spt.optracker.FilterOps(optracker.OperationUnpin, optracker.PhaseError) {
if _, ok := localpis[p.Cid().String()]; ok {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

Shouldn't it be !ok? If it's reported by IPFS, it's not been unpinned, thus it should not be Cleaned.

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

Good catch, thanks.

This comment has been minimized.

@hsanjuan

hsanjuan Jun 26, 2018

Collaborator

Means we need a test (probably missing in maptracker too?)

return fltops
}

func filterOpsMap(ops map[string]*Operation, filters []interface{}) map[string]*Operation {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

This seems unefficient. It loops the whole map for every filter. Instead of looping it once and check, for every element, if it matches one of the filters. I'm assuming that looping the map is generally more expensive than looping the filters, even if the final number of comparisons is the same.

I also don't see the advantage in using recursion here when a simple for over the filters solves the issue. It should be significantly cheaper than creating a whole new context to call the function, but mostly is about readability (there are not so many filter types).

This comment has been minimized.

@lanzafame

lanzafame Jun 26, 2018

Author Collaborator

This seems unefficient. It loops the whole map for every filter.

It may be inefficient but it isn't looping the whole map for every filter. It only loops the whole map once, and then only loops the filtered map for consecutive filters. Reading my godoc of the filter-related functions after not looking at them for a while, I realise they don't convey this at all so I shall change those.

Instead of looping it once and check, for every element, if it matches one of the filters.

This wouldn't allow for the usage seen in stateless.SyncAll where the map is filtered by an operation, i.e. Pin/Unpin, and then by phase, i.e. Error, essentially this function results in an intersection of the provided filters on the operation map.

I also don't see the advantage in using recursion here when a simple for over the filters solves the issue.

The reason why I went with recursion as it resulted in much more readable code and code climate complained about my initial non-recursive implementation.

This comment has been minimized.

@hsanjuan

hsanjuan Jun 26, 2018

Collaborator

Ah, I was reading this as an OR filter, not an AND. If the problem is filtering by operations and then by phase, we might as well use TrackerStatus as filter value directly

This comment has been minimized.

@lanzafame

lanzafame Jul 2, 2018

Author Collaborator

The issue with using a TrackerStatus is that then you can't get all errors or all pinOps regardless of phase.

This comment has been minimized.

@lanzafame

lanzafame Jul 2, 2018

Author Collaborator

I got halfway through implementing it to use TrackerStatus when I realised this.

This comment has been minimized.

@hsanjuan

hsanjuan Jul 2, 2018

Collaborator

Use -1 for all ?

This comment has been minimized.

@lanzafame

lanzafame Jul 2, 2018

Author Collaborator

But that would not be filtering anything?

This comment has been minimized.

@hsanjuan

hsanjuan Jul 2, 2018

Collaborator

Sorry, misread. Maybe making filter be a slice of slices. [][]TrackerStatus were {{status1, status2}, {status3}} means (Status1 OR status 2) AND (status3) ? Or simply two methods (filterOR, filterAND)

This comment has been minimized.

@lanzafame

lanzafame Jul 2, 2018

Author Collaborator

thoughts on this as a way of doing the filtering via trackerstatuses: https://play.golang.org/p/ykGXNsnderW

This comment has been minimized.

@lanzafame

lanzafame Jul 3, 2018

Author Collaborator

So I implemented this and it adds a lot of complexity that really outweighs the benefits of getting rid of the interface{} parameter. And it is actually more error prone because the conversion from TrackerStatus to OperationType and Phase isn't perfect. On top of that, the behaviour is less intuitive.

I am of the opinion that we leave it as is and if we have issues with this later, we tackle it as a separate thing to the stateless tracker. I am happy to push up the changes as separate PR?

// with the matching filter. Note, only supports
// filters of type OperationType or Phase, any other type
// will result in a nil slice being returned.
func (opt *OperationTracker) FilterOps(filters ...interface{}) []*Operation {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

Per comments above/below, FilterOps should not be necessary.


// Sync returns the updated local status for the given Cid.
func (spt *Tracker) Sync(c *cid.Cid) (api.PinInfo, error) {
return spt.Status(c), nil

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

Sync should do things similar to SyncAll right? Check if it's in error, and check if it should no longer be and clean up the operation then.

return nil, err
}

for _, p := range spt.optracker.FilterOps(optracker.OperationPin, optracker.PhaseError) {

This comment has been minimized.

@hsanjuan

hsanjuan Jun 25, 2018

Collaborator

Actually, the other way to do this (and which saves this atomic method) is to TrackNewOperation(cid, OperationPin, PhaseDone), followed by a call to Clean(op) if it returned a new op (see maptracker.syncStatus())).

@lanzafame lanzafame force-pushed the statelesstracker branch from 6e3de6c to 567e2c4 Jul 2, 2018

@hsanjuan hsanjuan added needs review and removed in progress labels Jul 3, 2018

@lanzafame lanzafame force-pushed the statelesstracker branch from 567e2c4 to e4baeaa Jul 4, 2018

@wafflebot wafflebot bot added the in progress label Jul 4, 2018

}

// New creates a new StatelessPinTracker.
func New(cfg *Config, pid peer.ID) *Tracker {

This comment has been minimized.

@hsanjuan

hsanjuan Jul 17, 2018

Collaborator

Did we say it would be better to pass the state? Serialization and de-serialization from the state is really going to hurt perf here.

This comment has been minimized.

@lanzafame

lanzafame Jul 20, 2018

Author Collaborator

I added a benchmark to the localStatus function, where the Cluster.Pins rpc call is made and the deserialization occurs, that way I can measure an improvement via passing in the State. I will do that in a separate PR.

}

// Enqueue puts a new operation on the queue, unless ongoing exists.
func (spt *Tracker) enqueue(c api.Pin, typ optracker.OperationType) error {

This comment has been minimized.

@hsanjuan

hsanjuan Jul 17, 2018

Collaborator

We should really try to have a base tracker that can be used by both to dedup all these lines. I don't feel good about having so many dup code lines. In a different PR.

logger.Error(err)
continue
}
p := api.PinInfo{

This comment has been minimized.

@hsanjuan

hsanjuan Jul 17, 2018

Collaborator

This is still missing

flag.Parse()

if len(customLogLvlFacilities) <= 0 {
for f := range ipfscluster.LoggingFacilities {

This comment has been minimized.

@hsanjuan

hsanjuan Jul 17, 2018

Collaborator

This introduces a dependency back to cluster from this package.

As a policy, components should not need to use the ipfscluster package for anything as they are fully independent and this can only complicate things in the future (create cycles, difficult extractions etc).

This also introduces code which is essentially a dup from the main package tests. My impression is that, since these tests are limited to this component you should not need any fine-tuning of the facilities (there is basically 1 only).

This comment has been minimized.

@lanzafame

lanzafame Jul 20, 2018

Author Collaborator

I want to split this test logging flags into a separate package, in a different PR, I will just remove it from here for now. Was only for debugging purposes anyway.

@lanzafame lanzafame force-pushed the statelesstracker branch from e4baeaa to 2b571c5 Jul 20, 2018

@lanzafame

This comment has been minimized.

Copy link
Collaborator Author

lanzafame commented Jul 20, 2018

@hsanjuan I think I have addressed all the outstanding comments.

@hsanjuan

This comment has been minimized.

Copy link
Collaborator

hsanjuan commented Jul 23, 2018

@lanzafame I had it in mind that some tests here were failing and I should have a look. Was this the case and the issues were fixed or it was simply not the case?

@lanzafame

This comment has been minimized.

Copy link
Collaborator Author

lanzafame commented Jul 24, 2018

I think there was but that was a while ago and is now fixed. Though now I have code climate issue 😑 🙃

@hsanjuan

This comment has been minimized.

Copy link
Collaborator

hsanjuan commented Jul 24, 2018

It's fine with me if not fixed, unless the fix improves things and is simple

lanzafame added some commits Jun 7, 2018

implements a stateless pintracker
Also updates to the optracker to make retrieving information easier.

License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
optracker: fix complexity of filter fn
License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>

lanzafame and others added some commits Jun 11, 2018

stateless: reduce num of methods
License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
uses new gorpc method to distinguish err type
License: MIT
Signed-off-by: Adrian Lanzafame <adrianlanzafame92@gmail.com>
Fix tests after rebase to master
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>

@hsanjuan hsanjuan force-pushed the statelesstracker branch from 2b571c5 to 584f532 Aug 14, 2018

@hsanjuan

This comment has been minimized.

Copy link
Collaborator

hsanjuan commented Aug 14, 2018

There are a couple of things left:

  • Allow switching between pintracker implementations when launching
  • Making sure stateless tracker implementation configuration section exists
  • Running tests with both implementations
  • Porting sharding changes to the maptracker to the stateless tracker

-- maybe for other PRs

  • Initializing stateless tracker with state
  • refactoring and extracting common code from both trackers

hsanjuan added some commits Aug 14, 2018

Remove unreachable code
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
Test with stateless tracker. Make map tracker default for tests.
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
Allow selecting pintracker with ipfs-cluster-service
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
Align stateless tracker with sharding
Also: Fixes #500

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>

@hsanjuan hsanjuan referenced this pull request Aug 14, 2018

Closed

Pintracker: Items stuck in error state #500

14 of 25 tasks complete
@hsanjuan

This comment has been minimized.

Copy link
Collaborator

hsanjuan commented Aug 14, 2018

@lanzafame: I can't request your review because it's your PR, but mind looking at the latest commits before we merge?

@lanzafame
Copy link
Collaborator Author

lanzafame left a comment

@hsanjuan I am happy with those changes.

@hsanjuan hsanjuan merged commit e86c2e6 into master Aug 15, 2018

6 checks passed

codeclimate All good!
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.5%) to 65.075%
Details

@wafflebot wafflebot bot removed the in progress label Aug 15, 2018

@hsanjuan hsanjuan deleted the statelesstracker branch Aug 15, 2018

@hsanjuan hsanjuan referenced this pull request Aug 19, 2018

Closed

Release 0.5.0 #454

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment