spotify / heroic

The Heroic Time Series Database
https://spotify.github.io/heroic/
Apache License 2.0
848 stars 109 forks source link

Analyse Heroic and user's perspective when hitting a timeout. Then implement necessary changes. #748

Open sming opened 3 years ago

sming commented 3 years ago

Implement Findings

Use Case Resolved: unknown Bigtable timeout & retry behaviour

Design & Implementation Notes

Concrete changes to make

  1. delete maxElapsedBackoffMs - it’s irrelevant for our Use Case (double check with Adam about his as it’s still in his doc)
  2. upgrade the Bigtable java lib from 1.12.1 to 1.18.1 and check that we see 2x retries before the 6s timeout (see Slack thread with Adam / testing Doc). We know we need this as it resolves 2 or 3 retry-related bugs.
  3. fine-tune all our TimeoutMs and TimeoutMillis settings
sming commented 3 years ago

we finally know how API clients will experience the BT Timeout :

  1. they’ll get a 200. Which is … interesting. And misleading IMO.
  2. they get no results. Well, at least for the query I was running.
  3. they get the following error message in the response body in errors[0].error : "Some fetches failed (100) or were cancelled (870), caused by Some fetches failed (100) or were cancelled (870)"
sming commented 3 years ago

we (Prism) need to decide if the above is acceptable. @malish8632 's argument is that this is the current behaviour and always has been, hence no changes are necessary.

My argument is that many more users who've never received a timeout before will now receive one, in the shape of a 200, which is super misleading cos it actually failed, in effect.

Hence the agreed-upon action is to phrase the 1, 2, 3 above as a "reminder" of how heroic returns requests that time out.