[improve] Use single buffer for metrics when noUnsafe use

zymap commented 5 hours ago

Motivation

Netty has a property io.netty.noUnsafe which allows disabling use the unsafe api. When this is enabled, the metrics won't collect because of the Unspport operation exception. The root cause is it calls the internalNioBuffer which only allow to call when the buffer count is 1. But when using the composite buffer the buffer count may more than 1. So use the buffer directly when the flag is enabled.

The exception: Caused by: java.lang.UnsupportedOperationException at io.netty.buffer.CompositeByteBuf.internalNioBuffer(CompositeByteBuf.java:1657) ~[io.netty-netty-buffer-4.1.113.Final.jar:4.1.113.Final] at io.netty.buffer.ByteBufUtil.writeUtf8(ByteBufUtil.java:924) ~[io.netty-netty-buffer-4.1.113.Final.jar:4.1.113.Final] at io.netty.buffer.ByteBufUtil.writeUtf8(ByteBufUtil.java:900) ~[io.netty-netty-buffer-4.1.113.Final.jar:4.1.113.Final] at io.netty.buffer.AbstractByteBuf.setCharSequence0(AbstractByteBuf.java:707) ~[io.netty-netty-buffer-4.1.113.Final.jar:4.1.113.Final]

Fixes #xyz

Main Issue: #xyz

PIP: #xyz

Motivation

Modifications

Verifying this change

[ ] Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

[ ] Dependencies (add or upgrade a dependency)
[ ] The public API
[ ] The schema
[ ] The default values of configurations
[ ] The threading model
[ ] The binary protocol
[ ] The REST endpoints
[ ] The admin CLI options
[ ] The metrics
[ ] Anything that affects deployment

Documentation

[ ] doc
[ ] doc-required
[x] doc-not-needed
[ ] doc-complete

Matching PR in forked repository

PR in forked repository:

github-actions[bot] commented 5 hours ago

@zymap Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

lhotari commented 4 hours ago

Netty has a property io.netty.noUnsafe which allows disabling use the unsafe api. When this is enabled, the metrics won't collect because of the Unspport operation exception.

The PR itself makes sense. However, there shouldn't be a need to ever disable the unsafe API when running Pulsar. It would cause a lot of performance issues if that's used. What would be the motivation for disabling the unsafe API?

apache / pulsar