Arvados

9550-keep-services-env @ acfc0f5

Review comments:

My bad. I got redmine email when Lucas changed the state and I mistook it as it is a reminder for me to review :(. I just realized it when I wanted to update the status as part of adding my comments. (In the future I should update the status when I start reviewing, not when I complete it ...). Anyways, since I already compiled all my comments, here I go.

arvadosclient.go

• Can you please update this comment to make the discovery part more explicit:
  "Use provided list of keep services, instead of using the ..." to something like "Use provided list of keep services, if any. (arvadosclient does not do a discovery to ...)"?

• Please update this comment

// Create a new ArvadosClient, initialized with standard Arvados environment
// variables ARVADOS_API_HOST, ARVADOS_API_TOKEN, and (optionally)
// ARVADOS_API_HOST_INSECURE.

discover.go

• Can you please cleanup the DiscoverKeepServers func a little to make it more readable with the new update

+       // Keep services are not provided in environment variable; Get keep services from api server
        var list svcList
-
-       // Get keep services from api server
        err := this.Arvados.Call("GET", "keep_services", "", "accessible", nil, &list)
        if err != nil {
                return err

discovery_test.go

• Are we using ExampleRefreshServices anywhere?

Misc

• Sorry for my lack of knowledge, but can you please explain why disk typed keep services cannot be listed here? If this is the case, do we need to enforce that only disk typed services are configured?

• Can you please add in all places where the env variable is being read that only non-disk typed keepservices are to be listed in this variable?

• Any documentation updates?

Some questions on my own:

• Shouldn't be the env var content be validated with a regex before being added to the service roots?
• On DiscoverKeepServers func, why is the uuid prefix hardcoded like that? 0000-bi6l4-xxxx
• Also on DiscoverKeepServers(): Wouldn't be cleaner code if the new conditional block assemble a list to be fed to loadKeepServers(list), so that the call to the API server can be on the "else" part of the conditional, avoiding to call SetServiceRoots directly?

Radhika Chippada wrote:

"Use provided list of keep services, instead of using the ..." to something like "Use provided list of keep services, if any. (arvadosclient does not do a discovery to ...)"?

Updated

[...]

Updated

• Can you please cleanup the DiscoverKeepServers func a little to make it more readable with the new update

Updated

• Are we using ExampleRefreshServices anywhere?

This is for godoc. I've renamed it to ExampleKeepClient_RefreshServices so it actually works now. See https://golang.org/pkg/testing/#hdr-Examples

• Sorry for my lack of knowledge, but can you please explain why disk typed keep services cannot be listed here? If this is the case, do we need to enforce that only disk typed services are configured?

Any type of service will work, but we treat them like non-disk services because we have no way of knowing what replication level they offer.

• Any documentation updates?

I think we should update the docs when both 9550 and 9551 are merged. Added a task. Any thoughts about where we should mention it?

Lucas Di Pentima wrote:

• Shouldn't be the env var content be validated with a regex before being added to the service roots?

Good point. Checking now with the URL parser. Also updated to ignore extra spaces, so " https://foo " doesn't get result in attempting "/abcdef0", "https://foo/abcdef0", and "/abcdef0".

• On DiscoverKeepServers func, why is the uuid prefix hardcoded like that? 0000-bi6l4-xxxx

It should look like a UUID so the rendezvous hash code doesn't get upset. I used 00000 as the fake site prefix (as opposed to zzzzz "for testing only"); bi6l4, the same infix used for real keep_service UUIDs; and a unique part that's unique.

• Also on DiscoverKeepServers(): Wouldn't be cleaner code if the new conditional block assemble a list to be fed to loadKeepServers(list), so that the call to the API server can be on the "else" part of the conditional, avoiding to call SetServiceRoots directly?

I considered that but it would mean adding a bunch of code to parse URL strings into KeepService structs, just so we can format them back to strings and check some conditions that we already know. And SetServiceRoots is a public method...

Looks good to me.