[
https://issues.apache.org/jira/browse/HDDS-16742?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andrey Yarovoy updated HDDS-16742:
----------------------------------
Description:
To populate the `LatestRequestType` tag,
`GrpcMetricsServerRequestInterceptor.onMessage` does:
{code:java}
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:java}
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.
was:
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.
> 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
> Assignee: Andrey Yarovoy
> Priority: Major
>
> To populate the `LatestRequestType` tag,
> `GrpcMetricsServerRequestInterceptor.onMessage` does:
> {code:java}
> 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:java}
> 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]
