orca: replace uint64 rps with double rps_fractional #54
Conversation
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
Signed-off-by: Mark D. Roth <roth@google.com>
|
I've updated the PR description to explain this. |
Signed-off-by: Mark D. Roth <roth@google.com>
|
|
||
| // Total QPS being served by an endpoint. This should cover all services that an endpoint is | ||
| // responsible for. | ||
| double qps = 6 [(validate.rules).double.gte = 0]; |
There was a problem hiding this comment.
I'm fine with the move to double, that makes sense for stats, but I'm less enthusiastic about switching from RPS to QPS terminology as the way to distinguish the fields. They are synonyms, and a large chunk of the world uses RPS, whereas Google uses QPS and Envoy / xDS has significant influence from this.
We should pick RPS / QPS as the xDS preferred standard, and put it in a style guide at the very least.
There was a problem hiding this comment.
I don't have any strong feeling about the preferred terminology, but given that the existing field here is called rps, we can't reuse that name without making a breaking change.
It might turn out that no one is actually using the existing field yet, in which case we could make a breaking change and no one would care. But we need this new field on a fairly tight timeline, so I'd prefer not to risk the need for a revert if we try it and encounter a problem later.
There was a problem hiding this comment.
As per our offline conversation, I've renamed qps to rps_fractional.
Signed-off-by: Mark D. Roth <roth@google.com>
markdroth
changed the title
Signed-off-by: Mark D. Roth roth@google.com
Need to represent this as a double to avoid rounding error in low-RPS services, which can greatly skew endpoint weights based on this value.
CC @yousukseung