Skip to content

Commit

Permalink
Global ratelimiter part 3: compact request-weighted-algorithm data (c…
Browse files Browse the repository at this point in the history
…adence-workflow#172)

Mostly-prerequisite for the final major step of building the global ratelimiter system in cadence-workflow/cadence#6141

This Thrift addition _does not_ need to be done, the system could instead exchange Protobuf / gob / JSON data.  But I've done it in Thrift because:
1. We already use Thrift rather heavily in service code, for long-term-stable data, like many database types.
2. We do not use Protobuf like ^ this _anywhere_.  This PR could begin to change that, but I feel like that has some larger ramifications to discuss before leaping for it.
3. Gob is _significantly_ larger than Thrift, and no more human-readable than Thrift or Protobuf, and it doesn't offer quite as strong protection against unintended changes (IDL files/codegen make that "must be stable" contract very explicit).

Notes otherwise include:
- i32 because more than 2 million operations within an update cycle (~3s planned) on a single host is roughly 1,000x beyond the size of ALL of our current ratelimits, and it uses half of the space of an i64.
  - To avoid roll-around issues even if this happens, the service code saturates at max-i32 rather than rolling around.  We'll just lose precise weight information across beyond-2m hosts if that happens.
- `double` is returned because it's scale-agnostic and not particularly worth squeezing further, and it allows the aggregator to be completely target-RPS-agnostic (it doesn't need limiters or that dynamic config _at all_ as it just tracks weight).
  - This could be adjusted to a... pair of ints?  Local/global RPS used, so callers can determine their weight locally?  I'm not sure if that'd be clearer or more useful, but it's an option, especially since I don't think we care about accurately tracking <1RPS (so ints are fine).
- If we decide we care a lot about data size, key strings are by far the majority of the bytes.  There are a lot of key-compaction options (most simply: a map per collection name), we could experiment a bit.

And last but not least, if we change our mind and want to move away from Thrift here:
we just need to make a new `any.ValueType` string to identify that new format, and maintain this thrift impl for as long as we want to allow transparent server upgrades.  And when we remove it, if someone still hasn't upgraded yet, they'll just fall back to local-only behavior (which is what we have used for the past several years) until the deploy finishes.  Risk is extremely low.
  • Loading branch information
Groxx authored and timl3136 committed Jul 16, 2024
1 parent 83d5cae commit 580ce81
Showing 1 changed file with 66 additions and 16 deletions.
82 changes: 66 additions & 16 deletions thrift/history.thrift
Original file line number Diff line number Diff line change
Expand Up @@ -377,29 +377,79 @@ struct GetFailoverInfoResponse {
}

struct RatelimitUpdateRequest {
// impl-specific data.
// likely some simple top-level keys and then either:
// - map<ratelimit-key-string, something>
// - list<something>
//
// this is a single blob rather than a collection to save on
// repeated serialization of the type name, and to allow impls
// to choose whatever structures are most-convenient for them.
/**
* impl-specific data.
*
* likely some simple top-level keys and then either:
* - map<ratelimit-key-string, something>
* - list<something>
*
* this is a single blob rather than a collection to save on
* repeated serialization of the type name, and to allow impls
* to choose whatever structures are most-convenient for them.
*/
10: optional shared.Any data
}

struct RatelimitUpdateResponse {
// impl-specific data.
// likely some simple top-level keys and then either:
// - map<ratelimit-key-string, something>
// - list<something>
//
// this is a single blob rather than a collection to save on
// repeated serialization of the type name, and to allow impls
// to choose whatever structures are most-convenient for them.
/**
* impl-specific data.
*
* likely some simple top-level keys and then either:
* - map<ratelimit-key-string, something>
* - list<something>
*
* this is a single blob rather than a collection to save on
* repeated serialization of the type name, and to allow impls
* to choose whatever structures are most-convenient for them.
*/
10: optional shared.Any data
}

/**
* first impl of ratelimiting data, collected by limiters and sent to aggregators.
*
* used in an Any with ValueType: WeightedRatelimitUsageAnyType
*/
struct WeightedRatelimitUsage {
/** unique, stable identifier of the calling host, to identify future data from the same host */
10: required string caller
/** milliseconds since last update call. expected to be on the order of a few seconds or less. */
20: required i32 elapsedMS
/** per key, number of allowed vs rejected calls since last update. */
30: required map<string, WeightedRatelimitCalls> calls
}

/** Any{ValueType} identifier for WeightedRatelimitUsage data */
const string WeightedRatelimitUsageAnyType = "cadence:loadbalanced:update_request"

/** fields are required to encourage compact serialization, zeros are expected */
struct WeightedRatelimitCalls {
/**
* number of allowed requests since last call.
* assumed to be <1m or so, saturates at MAX_INT32.
*/
10: required i32 allowed
/**
* number of rejected requests since last call.
* assumed to be <1m or so, saturates at MAX_INT32.
*/
20: required i32 rejected
}

/**
* first impl of ratelimiting data, result from aggregator to limiter.
*
* used in an Any with ValueType: WeightedRatelimitQuotasAnyType
*/
struct WeightedRatelimitQuotas {
/** RPS to allow per key */
10: required map<string,double> quotas
}

/** Any{ValueType} identifier for WeightedRatelimitQuotas data */
const string WeightedRatelimitQuotasAnyType = "cadence:loadbalanced:update_response"

/**
* HistoryService provides API to start a new long running workflow instance, as well as query and update the history
* of workflow instances already created.
Expand Down

0 comments on commit 580ce81

Please sign in to comment.