I only recently saw it but you might be able to just use this

Originally posted by @ChrisSchinnerl in #1 (comment)

It is still being used as the default sleep time in case we fail to fetch the size of the skyfile from skyd.

I don't particularly like this approach, so if someone has a suggestion for improving https://github.com/SkynetLabs/pinner/pull/7/files#diff-febdfba77f54946ad0c77a437167750d33fe4aa988fdd0c8935f2fd90cb84442R216, I'd be happy to have it. After we remove it from there, we can remove it completely.

Originally posted by @ro-tex in #7 (comment)

Aha I see you already have PINNER_LOG_FILE, so maybe PINNER_LOG_ENABLED?

Originally posted by @peterjan in #35 (comment)

NOTE: This issue depends on https://gitlab.com/SkynetLabs/skyd/-/issues/328 being implemented first.

this should be moved to skyd.
We should have an endpoint to return all skylinks.

Originally posted by @MSevey in #2 (comment)

Overview

We want the ability to ask the pinner to pin a skylink.

Ivaylo:

This needs to be further specified.

My suggested implementation is to introduce a new concept to Pinner which is unhealthy skylinks. This will be a flag on the given skylink which would require that it's pinned to an additional server, regardless of the current number of pinning servers. The flag will be lowered as soon as the skylink is pinned to a new server.

Matt:

agreed, should have a new flag. I would make it generic, like manualRequest instead of unhealthy because I could see this being used for highly important skylinks that we want extra redundancy on.

Design or Proposal

(lord of the nits) f/u You could make this a <-chan struct{} to prevent the receiver of the result from closing the channel.
ErrAvail would be a nice name I think. I thought we usually call them c so I had to double check but apparently we commonly use nice names.

Originally posted by @ChrisSchinnerl in #22 (comment)

Overview

The POST /sweep action is rather slow (~2.25 hours for ~145k files on dev1). The main reason for that is that we need to lock each skylink individually and then update it individually. That's both many function calls but mostly many calls to MongoDB.

What we can do about this is add an optional batch mode which we can use when sweeping. When using that we can either lock the files individually (safer) or all at once (potentially disrupts* other servers). Once we have the files locked, we can update and unlock them in batches of 100 or even more.

*: The potential disruption to other servers is important because it will not just delay them until it's done. If we lock all skylinks and another server checks for skylinks it needs to work with it won't find any and it will assume that it's done with its sweep/scan and it won't perform a new one until the following day or a manual request. This is potentially problematic and it should be avoided.

Design or Proposal

This is saying that there are no skylinks that can be pinned by this server right? Feels like we should just return the mongo.ErrNoDocuments in that case.

ErrSkylinkNoExist should only be used if we are trying to update a skylink record and that record doesn't exist.

Originally posted by @MSevey in #7 (comment)

Note: This also needs to happen in accounts.

f/u This is not very graceful because the goroutines might just be terminated when the test ends without making sure the http server actually shut down. One way to do that gracefully is something like this

tester.externShutdownDone = make(chan error)
go func() {
  tester.externShutdownErr = srv.ListenAndServe()
  close(tester.externShutdownDone)
}()

and then

func (t *Tester) Close() error {
  if err := t.staticSRV.Shutdown(someCtx); err != nil {
    return err
  }
  <-tester.externShutdownDone
  return tester.externShutdownErr
}

Originally posted by @ChrisSchinnerl in #1 (comment)

Not sure if I should have put the missing packages here or in integration-pkgs.

Originally posted by @m-cat in #30 (comment)

This is way too long. We should replace it with build.Retry. We should actually replace all of these sleeps with that.

Originally posted by @ro-tex in #30 (comment)

This is not a requirement for this MR but something we should keep in the back of our heads is exploring mongo change streams (https://www.mongodb.com/docs/manual/changeStreams/#watch-a-collection--database--or-deployment).

If we ever have settings which we want to apply asap, that might be a good way to do it without polling.

Originally posted by @ChrisSchinnerl in #17 (comment)

I agree with PJ on the naming. I think flipping it to pinned would be more readable.

Originally posted by @MSevey in #7 (comment)

you're holding this lock while doing a bunch of I/O, are you sure that's fine?

Originally posted by @peterjan in #7 (comment)

Overview

Additionally, we want the status endpoint to return information about start and finish times for the sweep, number of unknown and missing skylinks handled, etc.

https://github.com/SkynetLabs/pinner/pull/22/files#r875785159

Something else that is part of this effort is keeping track of all rebuilds across the cluster. Otherwise calling this endpoint will return unreliable information because the caller might hit a different server.

More thoughts: #22 (comment)

Design or Proposal

we should make the underpinned value an environment variable so that we can increase/decrease the redundancy easily.

Originally posted by @MSevey in #2 (comment)

Proposal

As we want this to be a cluster-wide setting, we cannot use an environment variable for it because each server will have its own value and they will need to do a complicated negotiation in order to decide on a common, cluster-wide number.

That's why we should add a configuration or settings collection in Mongo and use that to store all cluster-wide settings, similar to what we do in accounts when we need to disable registrations on a given cluster.

follow up; I would add a parseSkylink helper to ensure all endpoints and future endpoint parse the skylink the same way.

Originally posted by @MSevey in #7 (comment)

f/u

I think there was a misunderstanding. It's fine for this to be exposed as DefaultConfiguration for testing or whatever.
But we should never assign to global vars like we do in LoadConf.

Right now we got an unnecessary race condition in LoadConf when we call it from 2 threads because both are assigning to a global var. During a test which spins up 2 pinners this will fail with a race condition.

So we don't need func Conf() Config. Just a func LoadConf() (Config, error) { which creates a new Conf instance. That way we can call it a million times in parallel without the race detector going off because we are assigning to the same var.

Originally posted by @ChrisSchinnerl in #1 (comment)

Also: #1 (comment)