Andrey Yarovoy created HDDS-16742:
-------------------------------------
Summary: GrpcMetricsServerRequestInterceptor renders every request
to protobuf text format to read one field
Key: HDDS-16742
URL: https://issues.apache.org/jira/browse/HDDS-16742
Project: Apache Ozone
Issue Type: Improvement
Components: OM
Reporter: Andrey Yarovoy
To populate the `LatestRequestType` tag,
`GrpcMetricsServerRequestInterceptor.onMessage` does:
{code}
String[] messageFields = message.toString()
.split(System.lineSeparator());
// messageFields[0] should be in the format
// cmdType: "type"
String[] cmdTypeLine = messageFields[0].split(":");
// Get only the type and remove any leading spaces
grpcMetrics.setRequestType(cmdTypeLine[1].trim());
{code}
`AbstractMessage.toString()` is a full `TextFormat` render of the *entire*
request. On this server the
request is an `OMRequest`, so every call allocates a text rendering of the
whole message — including
nested key, ACL and list payloads — splits it into an array of lines, and then
uses exactly one line
before discarding all of it. For a large request this costs far more than
everything else the metrics
interceptors do combined.
`cmdType` is field 1 of `OMRequest` and is directly readable. Keeping the
interceptor generic over
`ReqT` (and avoiding a `hadoop-hdds/framework` →
`hadoop-ozone/interface-client` dependency), the
descriptor is enough:
{code}
Descriptors.FieldDescriptor cmdType =
parsedMessage.getDescriptorForType().findFieldByName("cmdType");
if (cmdType != null && parsedMessage.hasField(cmdType)) {
grpcMetrics.setRequestType(parsedMessage.getField(cmdType).toString());
}
{code}
The current parsing is also fragile: it assumes `cmdType` is rendered first,
that the value contains no
`:`, and that `messageFields[0]` splits into at least two parts. A request
whose first line is not
`cmdType: …` yields `cmdTypeLine` of length 1 and throws
`ArrayIndexOutOfBoundsException` from inside a
server interceptor. Reading the field removes all three assumptions.
Two related issues in the same area, in scope for this Jira since they concern
the same field:
- `GrpcMetrics.getMetrics()` is `synchronized` but `setRequestType()` is not,
so the synchronization
does not establish the happens-before it looks like it does for `requestType`.
- `requestType` is a single server-wide field, so `LatestRequestType` is
inherently whichever concurrent
call wrote last. That may be acceptable for a tag literally named "latest", but
it should be made
deliberate — at minimum `volatile` — rather than left as an unsynchronized race.
### Testing
Unit test on the interceptor with a mocked `ServerCall` / `ServerCallHandler`:
drive `onMessage` with an
`OMRequest` and assert `GrpcMetrics.getRequestType()` returns the `cmdType`
value, including for a
request whose other fields contain `:` and newlines, which the current string
parsing gets wrong.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
