Convert Datastore to GAPIC

[quartzmo]

This PR converts all Datastore requests to the GAX client introduced in #884.

There is a breaking change: The retries parameter is no longer supported (although method signatures have not changed.) See #848 and #849 for proposals to add new configuration options for retry behavior.

Also, Datastore is one of the three services that still requires expanded acceptance testing (#759). I could add the needed tests as part of this PR if desired.

[quartzmo]

This PR should also remove files that are no longer needed for backoff, to close #893.

[blowmage]

Not all gems are converted to GAX, so the backoff code should stay until they are all converted.

[quartzmo]

Oh right, I mistakenly thought it wasn't shared. Of course.

[quartzmo]

The fourth commit on this PR removes retries (no longer supported) from the factory methods, and adds a new client_config parameter accepting a hash of google-gax client_config values. Our intention with this change is to greatly increase client control over retry, timeout, and error handling behaviors, addressing issues such as #808.

Here is the default datastore_client_config.json, which can be overridden via the new parameter:

{
  "interfaces": {
    "google.datastore.v1.Datastore": {
      "retry_codes": {
        "retry_codes_def": {
          "idempotent": [
            "DEADLINE_EXCEEDED",
            "UNAVAILABLE"
          ],
          "non_idempotent": []
        }
      },
      "retry_params": {
        "default": {
          "initial_retry_delay_millis": 100,
          "retry_delay_multiplier": 1.3,
          "max_retry_delay_millis": 60000,
          "initial_rpc_timeout_millis": 60000,
          "rpc_timeout_multiplier": 1.0,
          "max_rpc_timeout_millis": 60000,
          "total_timeout_millis": 600000
        }
      },
      "methods": {
        "Lookup": {
          "timeout_millis": 60000,
          "retry_codes_name": "idempotent",
          "retry_params_name": "default"
        },
        "RunQuery": {
          "timeout_millis": 60000,
          "retry_codes_name": "idempotent",
          "retry_params_name": "default"
        },
        "BeginTransaction": {
          "timeout_millis": 60000,
          "retry_codes_name": "non_idempotent",
          "retry_params_name": "default"
        },
        "Commit": {
          "timeout_millis": 60000,
          "retry_codes_name": "non_idempotent",
          "retry_params_name": "default"
        },
        "Rollback": {
          "timeout_millis": 60000,
          "retry_codes_name": "non_idempotent",
          "retry_params_name": "default"
        },
        "AllocateIds": {
          "timeout_millis": 60000,
          "retry_codes_name": "non_idempotent",
          "retry_params_name": "default"
        }
      }
    }
  }
}

As you can see, there are some interesting fields, such as retry_codes, which suggests the ability to configure retry logic based on error status codes. At this time there is not much documentation for the client_config fields, but @jmuk from google-gax is working with us and should be able to answer questions.

Please note that this leaks the GAPIC implementation to the google-cloud-datastore public API. If google-gax changes its implementation, this may result in a change to the google-cloud-datastore public API.

@timanovsky @bmclean Any feedback on this proposal? We hope to merge and release this PR next week.

[bmclean]

@quartzmo Please correct me if I have any of this wrong:
For a datastore Lookup, since it has a retry_codes_name defined of "idempotent", if an UNAVAILABLE error was raised, it would retry using the default params.
For a datastore Commit, since it has a retry_codes_name defined of "non_idempotent", any error raised would not be retried.

[blowmage]

@bmclean I'll take a swing at answering your question. Yes, your understanding is correct. That is the default behavior. We asked several times which endpoints we should retry and which ones we should not. But we never got an answer so we did a blanket retry on them all. I believe that this new behavior is correct.

Of course, the good news is that you can now override how each endpoint is treated and use whatever behavior you wish. :)

[bmclean]

@blowmage What do you mean by "blanket retry on them all"? Retry all for a specific method as opposed to a specific endpoint?

[blowmage]

Correct. Not all errors, and not all endpoints should be retried. Of course, you are free to override the default configuration and make it behave as you want.

[bmclean]

Thanks. I agree that the proposed default config is correct. For Lookup and RunQuery, if datastore returns either a DEADLINE_EXCEEDED or UNAVAILABLE error the request is retried as per the retry configuration. For all other endpoints, the default is not to invoke the retry logic.

[blowmage]

This new configuration comes from Google. It has been hard for those of us outside of Google to get this information, so we are happy to finally get this level of guidance. 😄

[quartzmo]

@blowmage I believe I've updated this PR with everything it needs to match the merged GAPIC conversions for Vision and Pub/Sub. PTAL!

[quartzmo]

oops wrong comment button