Bug #7162
closed[SDKs] GoSDK supports any Keep service type, using proxy replication logic for non-disk types
Description
Functional requirements¶
- The Go SDK Keep client will talk to all accessible Keep services, regardless of their service type.
- The client will send the X-Keep-Desired-Replication header in all PUT requests, and respect the X-Keep-Replicas-Stored header in the response, regardless of service type
- The client only has one writer at a time writing to non-disk services, to avoid sending redundant requests (e.g., if it wants to 2 replicas of a block, it doesn't send the block to two gateways that will each replicate the block 2+ times).
- Use "disk" timeouts for all services that don't have type "proxy". (Choosing timeouts based on type is rather wrong anyway -- but until we have a better approach, "api told us to use a proxy" is still the best readily available indication that the network between us and the keep service is slow.)
Implementation¶
We don't get the "max replicas service X is able to store" information from the API server, but we'll move the code in that direction.KeepClient
gets a new attribute,replicasPerService int
.DiscoverKeepServers
setsreplicasPerService
to1
if all non-ReadOnly services haveSvcType=="disk"
, otherwise0
. (Note the current code forgets to resetUsing_proxy
tofalse
if the API server stops giving out proxies between one call and the next. We should at least avoid making that mistake again -- and possibly fix that Using_proxy bug, since it's trivial.)DiscoverKeepServers
adds all services to localRoots, not just disk and proxy. The only reason to look at SvcType is to choose whichthis.setClientSettings*
function to use.- switch service.SvcType { - case "disk": - localRoots[service.Uuid] = url - case "proxy": - localRoots[service.Uuid] = url - this.Using_proxy = true - } + localRoots[service.Uuid] = url + if service.SvcType == "proxy" { + this.Using_proxy = true + }
putReplicas
reduces the number of concurrent writer threads according toreplicasPerService
. The easiest way to do this is probably something like:- for active < remaining_replicas { + replicasPerThread := this.replicasPerService + if replicasPerThread < 1 { + // unlimited or unknown + replicasPerThread = remaining_replicas + } + + for active * replicasPerThread < remaining_replicas {
uploadToKeepServer
should ignore this.Using_proxy here:- if this.Using_proxy { req.Header.Add(X_Keep_Desired_Replicas, fmt.Sprint(this.Want_replicas)) - }
Updated by Brett Smith over 9 years ago
- Target version changed from Arvados Future Sprints to 2015-09-16 sprint
Updated by Brett Smith over 9 years ago
- Target version changed from 2015-09-16 sprint to 2015-09-30 sprint
Updated by Radhika Chippada over 9 years ago
- Status changed from New to In Progress
Updated by Tom Clegg over 9 years ago
Not sure about adding && arvadosclient.UUIDMatch(hint[2:])
to the "gateway" check. AFAICT it would only make a difference if the server has already told us about a keep service with a UUID that doesn't look like a UUID, in which case it would seem just as appropriate to use it as to ignore it. So it seems like an extra regexp match in every normal case, for the sake of changing from one reasonable outcome to another in an abnormal case...?
The other changes in SetServiceRoots seem to cause a hint like +XY{uuid}
to land us in the "fetch from gateway with given uuid" code, where X is any letter except A, and Y is anything. This seems incorrect: this code should only run for +K@{uuid}
. What is the intent/rationale for this change?
"type keepServices struct" should be "type keepService struct"
This looks like it could move out of the "for remaining_replicas > 0" loop:replicasPerThread := this.replicasPerService if replicasPerThread < 1 { // unlimited or unknown replicasPerThread = remaining_replicas }
The rest LGTM, thanks
Updated by Radhika Chippada over 9 years ago
- Reverted my updates to SetServiceRoots since this was wrong
- Renamed keepServices struct to keepService
- Moved replicasPerThread block as suggested.
Thanks.
Updated by Tom Clegg over 9 years ago
This comment doesn't seem to say anything that isn't obvious from the code:
// If there is error getting keep services, get list of keep disks if err != nil { if err := this.Arvados.List("keep_disks", nil, &m); err != nil {Suggestions:
- "Try getting keep_disks in case the API server is older than the change from keep_disks to keep_services."
- Maybe we should just delete this fallback (keep_services has been here since May 2014, #2776).
The rest LGTM, thanks.
Updated by Radhika Chippada over 9 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:b8ad249763a75f3eb7bea758b88cdc87389639f1.