Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Update to Vert.x 4.4.5 #387

Open
1 task done
pk-work opened this issue Sep 20, 2023 · 2 comments
Open
1 task done

[Feature]: Update to Vert.x 4.4.5 #387

pk-work opened this issue Sep 20, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@pk-work
Copy link
Contributor

pk-work commented Sep 20, 2023

Is there an existing issue for this?

  • I have searched the existing issues

The Problem

In Vert.x 4.4.5 all methods which exposes Netty classes directly are deprecated. NeonBee is using such methods at two places:

  1. ImmutableBuffer [1], where the ByteBuf class is used
  2. EventLoopHealthCheck [2], where EventExecutor is used.

Desired Solution

ImmutableBuffer: Could the approach below work?

// Current
    ImmutableBuffer(Buffer buffer) {
        this(requireNonNull(buffer), buffer.getByteBuf());
    }

    /**
     * Small optimization, as calling {@link Buffer#getByteBuf} will duplicate the underlying buffer.
     *
     * @param buffer     the buffer to wrap
     * @param byteBuffer the associated Netty byte-buffer
     */
    private ImmutableBuffer(Buffer buffer, ByteBuf byteBuffer) {
        // if the underlying byte buffer is read-only already, there is no need to make it any more immutable
        this.buffer = byteBuffer.isReadOnly() ? buffer : Buffer.buffer(byteBuffer.asReadOnly());
    }

// new
    ImmutableBuffer(Buffer buffer) {
        this.buffer = buffer instanceOf ImmutableBuffer ? buffer : Buffer.buffer(buffer);
    }

EventLoopHealthCheck: I'd suggest to use SuppressWarnings here because:

  • The long term solution is, that the metric "pending tasks" will be part of Vert.x Metrics. I talked already with Julien.
  • In Vert.x 5 we can use VertxInternal to access the EventExecutor. Of course this is an internal API, but the unofficial agreement is that we won't touch this API until the pending tasks metric is part of Vert.x Metrics.

[1]

this.buffer = byteBuffer.isReadOnly() ? buffer : Buffer.buffer(byteBuffer.asReadOnly());

[2]
for (EventExecutor elg : neonBee.getVertx().nettyEventLoopGroup()) {

Alternative Solutions

No response

Additional Context

No response

@pk-work pk-work added the enhancement New feature or request label Sep 20, 2023
@kristian
Copy link
Member

The second is easy, thanks that you talked to Julien, I think adding this to the Vert.x metrics is a very good addition and suppressing warnings on our side until then is fine for me 👍

In regards to the ImmutableBuffer, if there is no more possibility to get access to the underlying byte buffer in Vert.x 4.4.5, copying the Buffer either via Buffer.buffer(buffer) or calling Buffer.copy() would work, yes. However if in 4.4.5 getByteBuf only gets deprecated, we have to carry on the deprecation until the method is removed in a future minor or major release, as otherwise one could violate the immutability by simply calling the (now deprecated) getByteBuf method and accessing the underlying (mutable) ByteBuf directly. So I would suggest we keep the method for the time being until we can remove it after the getByteBuf method is gone?

@pk-work
Copy link
Contributor Author

pk-work commented Sep 25, 2023

Okay, so the plan is to annotate both of them with @SuppressWarnings, isn't it? @kristian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants