Bug #7162

[SDKs] GoSDK supports any Keep service type, using proxy replication logic for non-disk types

Added by Brett Smith over 5 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
SDKs
Target version:
Start date:
08/28/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0

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 sets replicasPerService to 1 if all non-ReadOnly services have SvcType=="disk", otherwise 0. (Note the current code forgets to reset Using_proxy to false 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 which this.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 to replicasPerService. 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))
    -    }
    

Subtasks

Task #7351: Review branch 7162-support-service-typesResolvedRadhika Chippada


Related issues

Related to Arvados - Feature #7159: [Keep] Implement an Azure blob storage volume in keepstoreResolved08/28/2015

Related to Arvados - Bug #7180: [Keep] [SDKs] Logic about which services to try should not reference service_typeClosed09/01/2015

Associated revisions

Revision b8ad2497
Added by Radhika Chippada over 5 years ago

closes #7162
Merge branch '7162-support-service-types'

History

#1 Updated by Peter Amstutz over 5 years ago

  • Story points set to 1.0

#2 Updated by Brett Smith over 5 years ago

  • Target version changed from Arvados Future Sprints to 2015-09-16 sprint

#3 Updated by Brett Smith over 5 years ago

  • Target version changed from 2015-09-16 sprint to 2015-09-30 sprint

#4 Updated by Brett Smith over 5 years ago

  • Assigned To set to Radhika Chippada

#5 Updated by Brett Smith over 5 years ago

  • Story points changed from 1.0 to 2.0

#6 Updated by Radhika Chippada over 5 years ago

  • Status changed from New to In Progress

#7 Updated by Tom Clegg over 5 years ago

  • Description updated (diff)

#8 Updated by Tom Clegg over 5 years ago

  • Description updated (diff)

#9 Updated by Tom Clegg over 5 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

#10 Updated by Radhika Chippada over 5 years ago

  • Reverted my updates to SetServiceRoots since this was wrong
  • Renamed keepServices struct to keepService
  • Moved replicasPerThread block as suggested.

Thanks.

#11 Updated by Tom Clegg over 5 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.

#12 Updated by Radhika Chippada over 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:b8ad249763a75f3eb7bea758b88cdc87389639f1.

Also available in: Atom PDF