-
Notifications
You must be signed in to change notification settings - Fork 23
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
Remove HostIP and SiamuxAddr from ContractMetadata #1712
Remove HostIP and SiamuxAddr from ContractMetadata #1712
Conversation
internal/upload/uploader/uploader.go
Outdated
logger: l, | ||
|
||
// static | ||
hk: c.HostKey, | ||
siamuxAddr: c.SiamuxAddr, | ||
siamuxAddr: siamuxAddr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if SiamuxAddr
is even used? If not, you can remove the field and avoid fetching the host here. You also don't need the HostStore
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field can be removed indeed but removing the host store is a little tricky because we have a retry-on-error loop that tries to refresh a contract in case it got renewed while an upload was taking place. We can make it work though because we could pass more information along with refreshUploaders
so we don't need the store in that flow. In the error flow we could do a partial refresh of the uploader's contract ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the siamux addr for this part?
return &Uploader{
...
host: hm.Host(c.HostKey, c.ID, c.SiamuxAddr),
...
}
refreshUploaders is called by newUpload which is called by UploadPackedSlab and UploadShards. Maybe I'm missing something but I couldn't find any host info in that call stack, so it seems like we'll have to get info from the host store at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, we do have to get that info at some point, but it's better to get all the host info at once as opposed to fetching on the fly every time. I would probably add
type hostContract struct {
api.ContractMetadata
api.HostInfo
}
which we manually construct as high up as possible. Where we fetch contracts we can fetch usable hosts and zip those two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterjan I'd propose that we keep this as-is for now. Sure we need to fetch the host now too every time we previously only fetched the contract but imo it's not worth the refactor to worry about that now.
After the hardfork we will be able to tear all of that contract renewal safety out anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ok. I knew more or less how this could be implemented because I dove into it a bit for RHP4 uploads. Matter of fact the RHP4 uploads PR does introduce this combined type and in the process I also had to get rid of the HostIP
and SiamuxAddr
.
21479c1
to
943a904
Compare
@@ -0,0 +1,9 @@ | |||
--- | |||
default: minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this is a major change since it breaks the API.
Implement #1691