[NebulousLabs/Sia] Persist renter cache settings, and renter bandwidth limiter settings (#3052)

lukechampine commented on this pull request.

> +func (r *Renter) updatePersistVersionFrom040To133() error {
+ metadata := persist.Metadata{
+ Header: settingsMetadata.Header,
+ Version: persistVersion040,
+ }
+ err := persist.LoadJSON(metadata, &r.persist, filepath.Join(r.persistDir, PersistFilename))
+ if err != nil {
+ return err
+ }
+ metadata.Version = persistVersion133
+ r.persist.MaxDownloadSpeed = DefaultMaxDownloadSpeed
+ r.persist.MaxUploadSpeed = DefaultMaxUploadSpeed
+ r.persist.StreamCacheSize = DefaultStreamCacheSize
+ return persist.SaveJSON(metadata, r.persist, filepath.Join(r.persistDir, PersistFilename))

I don’t see the issue with passing the persist path. It makes sense when you think of the function as rewriting a specific file. As for setting defaults, I can see why it might feel fragile to you. «Persist» can refer to both the file on disk and a field in the renter, so it makes sense that when you upgrade one, you should upgrade the other in tandem.

I think this might just be a problem with the name. In other modules, we’ve called this sort of function `convert`. In the contractor, there’s a `convertPersist` function that converts old contractor.json to the new format. Similarly, in the wallet, there’s a `convertPersistFrom112To120` function that converts the old wallet.json to a wallet.db. (Both of these functions take filenames as arguments.) «Convert» communicates the intent of the function clearly: it converts one thing into another thing. «Upgrade» is a more ambiguous name, especially when it’s a method of the renter instead of a standalone function.

I suggest that we rename the function to `convertPersistFrom040To133` and pass it the filename as an argument. Then it will match the persist conversion functions in the contractor and wallet. Alternatively, we could think of a better name that makes it more obvious that the function also sets fields in the renter.

Добавить комментарий