[
https://issues.apache.org/jira/browse/HDDS-16740?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andrey Yarovoy updated HDDS-16740:
----------------------------------
Description:
`GrpcMetrics` keeps its two per-call latency counters as `@Metric MutableRate`
fields —
`grpcQueueTime` and `grpcProcessingTime` in
`hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/grpc/metrics/GrpcMetrics.java`.
Both are
updated from `GrpcMetricsServerRequestInterceptor.onComplete()`, i.e. on every
gRPC call served by
`GrpcOzoneManagerServer`:
{code:java}
grpcMetrics.addGrpcQueueTime(queueTime);
grpcMetrics.addGrpcProcessingTime(processingTime);
{code}
`MutableRate extends MutableStat`, and `MutableStat.add(long)` is
`synchronized(this)`, so every gRPC
server thread serialises on the same two monitors. This is the same contention
class as HDDS-16304
(`RpcMetrics` on the Hadoop IPC fork) and HDDS-9377 (`OMLockMetrics` /
`PerformanceMetrics`), on the
path S3 Gateway uses to reach OM. The interceptors are installed
unconditionally in
`GrpcOzoneManagerServer`, and `ozone.om.s3.grpc.server_enabled` defaults to
`true`, so this path is
live on a default cluster.
Replace both with the existing lock-free
`org.apache.hadoop.ozone.util.ConcurrentMutableRate`, which
accumulates into `LongAdder` / `LongAccumulator` and drains lazily on
`snapshot()` / `lastStat()`.
Why this is smaller than HDDS-16304
`GrpcMetrics` already implements `MetricsSource` and already owns
`getMetrics()`, so the hybrid-source
plumbing HDDS-16304 had to introduce is here for free. The change is:
- drop `@Metric` from the two fields and make them `final
ConcurrentMutableRate`, constructed in the
constructor;
- add two `snapshot(builder, all)` calls to the existing `getMetrics()`, next
to the current
`registry.snapshot(...)`;
- change the two package-private getters' return type (they have no callers in
the tree).
Everything else — the five `MutableCounterLong` fields, the registry, the
`MutableQuantiles` arrays and
their `registry.newQuantiles(...)` creation, the second record carrying the
`LatestRequestType` tag — is
untouched.
*Emitted names must not change*
`MutableMetricsFactory.getName(Field)` is
`StringUtils.capitalize(field.getName())` and
`@Metric.always()` defaults to `false`, so the fields currently emit
`GrpcQueueTimeNumOps` /
`GrpcQueueTimeAvgTime` and `GrpcProcessingTimeNumOps` /
`GrpcProcessingTimeAvgTime`, with no
stddev/min/max. `new ConcurrentMutableRate("GrpcQueueTime", "Queue time",
false)` and
`("GrpcProcessingTime", "Processing time", false)` reproduce that exactly.
There are no in-tree
consumers of these names (unlike the `RpcMetrics` ones, which `ozone insight`
reads), but operator
dashboards key on them, so they stay byte-identical.
Documented `ConcurrentMutableStat` caveat: a batched drain loses within-batch
variance, so stddev is
slightly underestimated under concurrency. Nothing in the tree reads the stddev
of these two rates.
*Scope*
In scope: the two `MutableRate` fields in `GrpcMetrics`.
Out of scope: the `MutableCounterLong` counters (already atomic) and the
`MutableQuantiles` arrays.
No wire or RPC protocol change, no configuration change, no new dependency.
*Testing*
Unit test in `hadoop-hdds/framework`, following
`hadoop-hdds/common/src/test/java/org/apache/hadoop/ipc_/metrics/TestRpcMetrics.java`
from HDDS-16304:
read the real emitted record through `MetricsCollectorImpl` and pin the four
rate names and their
values, that the registry-backed counters and the `LatestRequestType` tag still
appear, and an exact
sample count under concurrent `addGrpcProcessingTime`. Name parity can be
proven the same way it was
for HDDS-16304 — revert the main file to `git show HEAD:` with the new test in
place and confirm only
the assertions about the new wiring fail.
was:
`GrpcMetrics` keeps its two per-call latency counters as `@Metric MutableRate`
fields —
`grpcQueueTime` and `grpcProcessingTime` in
`hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/grpc/metrics/GrpcMetrics.java`.
Both are
updated from `GrpcMetricsServerRequestInterceptor.onComplete()`, i.e. on every
gRPC call served by
`GrpcOzoneManagerServer`:
{code}
grpcMetrics.addGrpcQueueTime(queueTime);
grpcMetrics.addGrpcProcessingTime(processingTime);
{code}
`MutableRate extends MutableStat`, and `MutableStat.add(long)` is
`synchronized(this)`, so every gRPC
server thread serialises on the same two monitors. This is the same contention
class as HDDS-16304
(`RpcMetrics` on the Hadoop IPC fork) and HDDS-9377 (`OMLockMetrics` /
`PerformanceMetrics`), on the
path S3 Gateway uses to reach OM. The interceptors are installed
unconditionally in
`GrpcOzoneManagerServer`, and `ozone.om.s3.grpc.server_enabled` defaults to
`true`, so this path is
live on a default cluster.
Replace both with the existing lock-free
`org.apache.hadoop.ozone.util.ConcurrentMutableRate`, which
accumulates into `LongAdder` / `LongAccumulator` and drains lazily on
`snapshot()` / `lastStat()`.
### Why this is smaller than HDDS-16304
`GrpcMetrics` already implements `MetricsSource` and already owns
`getMetrics()`, so the hybrid-source
plumbing HDDS-16304 had to introduce is here for free. The change is:
- drop `@Metric` from the two fields and make them `final
ConcurrentMutableRate`, constructed in the
constructor;
- add two `snapshot(builder, all)` calls to the existing `getMetrics()`, next
to the current
`registry.snapshot(...)`;
- change the two package-private getters' return type (they have no callers in
the tree).
Everything else — the five `MutableCounterLong` fields, the registry, the
`MutableQuantiles` arrays and
their `registry.newQuantiles(...)` creation, the second record carrying the
`LatestRequestType` tag — is
untouched.
### Emitted names must not change
`MutableMetricsFactory.getName(Field)` is
`StringUtils.capitalize(field.getName())` and
`@Metric.always()` defaults to `false`, so the fields currently emit
`GrpcQueueTimeNumOps` /
`GrpcQueueTimeAvgTime` and `GrpcProcessingTimeNumOps` /
`GrpcProcessingTimeAvgTime`, with no
stddev/min/max. `new ConcurrentMutableRate("GrpcQueueTime", "Queue time",
false)` and
`("GrpcProcessingTime", "Processing time", false)` reproduce that exactly.
There are no in-tree
consumers of these names (unlike the `RpcMetrics` ones, which `ozone insight`
reads), but operator
dashboards key on them, so they stay byte-identical.
Documented `ConcurrentMutableStat` caveat: a batched drain loses within-batch
variance, so stddev is
slightly underestimated under concurrency. Nothing in the tree reads the stddev
of these two rates.
### Scope
In scope: the two `MutableRate` fields in `GrpcMetrics`.
Out of scope: the `MutableCounterLong` counters (already atomic) and the
`MutableQuantiles` arrays.
No wire or RPC protocol change, no configuration change, no new dependency.
### Testing
Unit test in `hadoop-hdds/framework`, following
`hadoop-hdds/common/src/test/java/org/apache/hadoop/ipc_/metrics/TestRpcMetrics.java`
from HDDS-16304:
read the real emitted record through `MetricsCollectorImpl` and pin the four
rate names and their
values, that the registry-backed counters and the `LatestRequestType` tag still
appear, and an exact
sample count under concurrent `addGrpcProcessingTime`. Name parity can be
proven the same way it was
for HDDS-16304 — revert the main file to `git show HEAD:` with the new test in
place and confirm only
the assertions about the new wiring fail.
> Convert GrpcMetrics latency counters to lock-free ConcurrentMutableRate
> -----------------------------------------------------------------------
>
> Key: HDDS-16740
> URL: https://issues.apache.org/jira/browse/HDDS-16740
> Project: Apache Ozone
> Issue Type: Improvement
> Components: Ozone Manager
> Reporter: Andrey Yarovoy
> Priority: Major
>
> `GrpcMetrics` keeps its two per-call latency counters as `@Metric
> MutableRate` fields —
> `grpcQueueTime` and `grpcProcessingTime` in
> `hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/grpc/metrics/GrpcMetrics.java`.
> Both are
> updated from `GrpcMetricsServerRequestInterceptor.onComplete()`, i.e. on
> every gRPC call served by
> `GrpcOzoneManagerServer`:
> {code:java}
> grpcMetrics.addGrpcQueueTime(queueTime);
> grpcMetrics.addGrpcProcessingTime(processingTime);
> {code}
> `MutableRate extends MutableStat`, and `MutableStat.add(long)` is
> `synchronized(this)`, so every gRPC
> server thread serialises on the same two monitors. This is the same
> contention class as HDDS-16304
> (`RpcMetrics` on the Hadoop IPC fork) and HDDS-9377 (`OMLockMetrics` /
> `PerformanceMetrics`), on the
> path S3 Gateway uses to reach OM. The interceptors are installed
> unconditionally in
> `GrpcOzoneManagerServer`, and `ozone.om.s3.grpc.server_enabled` defaults to
> `true`, so this path is
> live on a default cluster.
> Replace both with the existing lock-free
> `org.apache.hadoop.ozone.util.ConcurrentMutableRate`, which
> accumulates into `LongAdder` / `LongAccumulator` and drains lazily on
> `snapshot()` / `lastStat()`.
>
> Why this is smaller than HDDS-16304
> `GrpcMetrics` already implements `MetricsSource` and already owns
> `getMetrics()`, so the hybrid-source
> plumbing HDDS-16304 had to introduce is here for free. The change is:
> - drop `@Metric` from the two fields and make them `final
> ConcurrentMutableRate`, constructed in the
> constructor;
> - add two `snapshot(builder, all)` calls to the existing `getMetrics()`,
> next to the current
> `registry.snapshot(...)`;
> - change the two package-private getters' return type (they have no callers
> in the tree).
> Everything else — the five `MutableCounterLong` fields, the registry, the
> `MutableQuantiles` arrays and
> their `registry.newQuantiles(...)` creation, the second record carrying the
> `LatestRequestType` tag — is
> untouched.
>
> *Emitted names must not change*
> `MutableMetricsFactory.getName(Field)` is
> `StringUtils.capitalize(field.getName())` and
> `@Metric.always()` defaults to `false`, so the fields currently emit
> `GrpcQueueTimeNumOps` /
> `GrpcQueueTimeAvgTime` and `GrpcProcessingTimeNumOps` /
> `GrpcProcessingTimeAvgTime`, with no
> stddev/min/max. `new ConcurrentMutableRate("GrpcQueueTime", "Queue time",
> false)` and
> `("GrpcProcessingTime", "Processing time", false)` reproduce that exactly.
> There are no in-tree
> consumers of these names (unlike the `RpcMetrics` ones, which `ozone insight`
> reads), but operator
> dashboards key on them, so they stay byte-identical.
> Documented `ConcurrentMutableStat` caveat: a batched drain loses within-batch
> variance, so stddev is
> slightly underestimated under concurrency. Nothing in the tree reads the
> stddev of these two rates.
>
> *Scope*
> In scope: the two `MutableRate` fields in `GrpcMetrics`.
> Out of scope: the `MutableCounterLong` counters (already atomic) and the
> `MutableQuantiles` arrays.
> No wire or RPC protocol change, no configuration change, no new dependency.
>
> *Testing*
> Unit test in `hadoop-hdds/framework`, following
> `hadoop-hdds/common/src/test/java/org/apache/hadoop/ipc_/metrics/TestRpcMetrics.java`
> from HDDS-16304:
> read the real emitted record through `MetricsCollectorImpl` and pin the four
> rate names and their
> values, that the registry-backed counters and the `LatestRequestType` tag
> still appear, and an exact
> sample count under concurrent `addGrpcProcessingTime`. Name parity can be
> proven the same way it was
> for HDDS-16304 — revert the main file to `git show HEAD:` with the new test
> in place and confirm only
> the assertions about the new wiring fail.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
