Story #7710

[SDKs] GoSDK KeepClient works with all service types, applying rules for the proxy type

Added by Brett Smith about 4 years ago. Updated about 4 years ago.

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

100%

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

Description

The Keep client in the Go SDK should connect to services of any type. For any type that isn't disk, it should follow the rules that we currently follow when connecting to proxies:

  • Only allow one writer thread at a time.
  • Use "proxy" timeouts.
  • Respect the replication headers in the response.
Test case:
  • Create some keep_services with service_type="fancynewblobstore"
  • Ensure those keep services are used when reading and writing
  • Ensure that the client only sends one write when the service reports that it wrote enough replicas

Write the above test cases to ensure proper behavior for non-disk, non-proxy services, then make whatever code changes are necessary to make them pass. Right now, we believe no code changes are necessary, but the tests will verify that.


Subtasks

Task #7874: Review branch: 7710-keepclient-all-service-typesResolvedRadhika Chippada


Related issues

Related to Arvados - Story #7696: [SDKs] PySDK KeepClient works with all service types, applying rules for the proxy typeResolved11/11/2015

Associated revisions

Revision 7ed2f9d8
Added by Radhika Chippada about 4 years ago

closes #7710
Merge branch '7710-keepclient-all-service-types'

Revision 8c2f3c1f
Added by Radhika Chippada about 4 years ago

refs #7710
Merge branch '7710-add-go-sdk-crunchrunner'

Revision 8c2f3c1f
Added by Radhika Chippada about 4 years ago

refs #7710
Merge branch '7710-add-go-sdk-crunchrunner'

History

#1 Updated by Tom Clegg about 4 years ago

Looks like the "unknown service type" handling is already correct.

        if service.ReadOnly == false {
            writableLocalRoots[service.Uuid] = url
            if service.SvcType != "disk" {
                this.replicasPerService = 0
            }
        }
So, maybe we just need
  • use proxy timeouts if any non-disk (instead of "if any proxy")
  • tests

#2 Updated by Radhika Chippada about 4 years ago

  • Status changed from New to In Progress
  • Assigned To set to Radhika Chippada
  • Target version changed from Arvados Future Sprints to 2015-12-02 sprint

#3 Updated by Tom Clegg about 4 years ago

We still seem to set Using_proxy to true and false in various places, but we never read it, so presumably we can just delete all references to it?
  • $ git grep Using_proxy
    sdk/go/keepclient/discover.go:  this.Using_proxy = false
    sdk/go/keepclient/discover.go:                  this.Using_proxy = true
    sdk/go/keepclient/keepclient.go:        Using_proxy        bool
    sdk/go/keepclient/keepclient.go:                Using_proxy:   false,
    sdk/go/keepclient/keepclient_test.go:   kc.Using_proxy = true
    sdk/go/keepclient/keepclient_test.go:   kc.Using_proxy = true
    services/datamanager/datamanager_test.go:               Using_proxy:   true,
    services/keepproxy/keepproxy_test.go:   kc.Using_proxy = true
    services/keepstore/keepstore.go:                Using_proxy:   true,
    services/keepstore/pull_worker_integration_test.go:             Using_proxy:   true,
    
These assertions are not effective -- the nil value of a string is "", not nil, so the assertion will pass even if the map doesn't have the desired entry. Should be Not(Equals), "" ...?
  •     c.Assert(kc.LocalRoots()[blobKeepService["uuid"].(string)], NotNil)
    

Pushed 5f7c3a3 to get some test coverage (admittedly not much) on keep_services#accessible.

#4 Updated by Tom Clegg about 4 years ago

Related: datamanager errors out if it sees a service_type other than "disk". The easiest way to fix this for typical cases also gives us an interim solution for #7853:
  • Make the "disk" string configurable with a command line argument (-service-type=X, default "disk")
  • If other service types are encountered, log that they're being ignored, but then continue.
  • Error out only if there are no services with the configured type.

#5 Updated by Radhika Chippada about 4 years ago

We still seem to set Using_proxy to true and false in various places, but we never read it, so presumably we can just delete all references to it?

Removed all these references. All the tests seem to be passing (still running tests). However, do we need to set ArvadosClient.External in any of these places?

These assertions are not effective -- the nil value of a string is "", not nil

Thanks for noticing. Updated these.

Related: datamanager errors out if it sees a service_type other than "disk".

Added an extra service-type arg with default value "disk" in keep.go and used it instead.

#6 Updated by Tom Clegg about 4 years ago

No need to write the default in the help message, the flag package does it for us:
  •   -service-type string
            Supported keepservice type. Default is disk. (default "disk")
    
  • "Supported" is a bit mysterious too; suggest "Operate only on keep_services with the specified service_type, ignoring all others."

The words "indexable" and "supported" seem to be used interchangeably in the names "indexableKeepServers" and "supportedServiceType". Perhaps they could just be called "selectedKeepServers" and "selectedServiceType", or even just "keepServers" and "serviceType"?

Likewise, the word "unsupported" in "Ignore unsupported service type: %v" seems weird (I'd expect "unsupported" to mean "datamanager can't handle this service_type"). How about:
  • "Skipping keep_service %q because its service_type %q does not match -service-type=%q"
This comment is superfluous -- it's already obvious from the code.
  • // Ignore any services that are of type other than the "supportedServiceType".
    // If no services of the "supportedServiceType" are found, raise an error.
    

#7 Updated by Tom Clegg about 4 years ago

Noting we don't have one of the tests requested by the issue:

Ensure that the client only sends one write when the service reports that it wrote enough replicas

(This shouldn't hold up the current branch, but still seems like a good idea. Perhaps we already have a test like this attached to the "max replicas expected per PUT" code?)

#8 Updated by Brett Smith about 4 years ago

  • Target version changed from 2015-12-02 sprint to 2015-12-16 sprint

#9 Updated by Radhika Chippada about 4 years ago

  • Made all those text updates for -service-type
  • Test "Ensure that the client only sends one write when the service reports that it wrote enough replicas". TestMakeKeepClientWithNonDiskTypeService asserts that kc.replicasPerService is set correctly, though there is room for more tests.

#10 Updated by Tom Clegg about 4 years ago

LGTM @ 311993c

There aren't any merge conflicts here, so doing "git rebase master" on this branch before merging it will eliminate some "merge master" noise in the git history.

#11 Updated by Radhika Chippada about 4 years ago

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

Applied in changeset arvados|commit:7ed2f9d875d69b1494372c0cb790b18187dbacf2.

#12 Updated by Radhika Chippada about 4 years ago

  • Target version changed from 2015-12-16 sprint to 2015-12-02 sprint

Also available in: Atom PDF