You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by steven lu <lu...@gmail.com> on 2023/01/29 11:57:13 UTC
[DISCUSS] PIP-244: Refactor ByteBuf release method
[DISCUSS] PIP-244: Refactor ByteBuf release method
Hello everyone. I hope you guys are all doing well. I would like to start
the discussion for PIP-244 https://github.com/apache/pulsar/issues/19350,
Please let me know if you have any concerns or questions.
------- Paste original PIP content to help quote ------
### Motivation
It may throw an exception when release a `ByteBuf` object. so the exception
in `ByteBuf.release` should be checked.
### Goal
Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()` in
all Pulsar's classes.
### API Changes
_No response_
### Implementation
1. Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()`.
Re: [DISCUSS] PIP-244: Refactor ByteBuf release method
Posted by Dave Fisher <wa...@comcast.net>.
Sent from my iPhone
> On Jan 30, 2023, at 1:21 AM, Shiji Lu <lu...@apache.org> wrote:
>
> Good suggestions,
> There are three ways to write release in current pulsar and bookkeeper
> projects:
> 1.ByteBuf.release: Call release directly without handling any exceptions;
> 2.ReferenceCountUtil.release: return processing status;
> 3. ReferenceCountUtil. safeRelease: without return value, print exception information
>
> I think one specification can be used at present, all of which are released in the way of ReferenceCountUtil.release, replacing these three ways
These methods may be logically the same. They will not perform the same. Pulsar is highly performant and able to process millions of messages per second. Please be sure you understand performance impacts of your proposal under heavy load.
Best,
Dave
>
>
>
>> On 2023/01/30 08:11:39 Enrico Olivelli wrote:
>> There is an open discussion on the Apache BooKeeper mailing list about
>> this kind of refactoring.
>>
>> I appreciate this initiative because I see it as a tentative to reduce
>> the tech debt.
>>
>> I think that it is pretty dangerous to do this kind of change.
>> Pulsar uses refcounting in a very advanced way and there are many
>> subtle cases that deserve careful handling.
>>
>> I like the motto "Don't break things that aren't broken", this is one
>> of the guiding principles while dealing with big open source projects
>> and communities as Apache Pulsar
>>
>>
>> Enrico
>>
>>> Il giorno lun 30 gen 2023 alle ore 03:28 Yunze Xu
>>> <yz...@streamnative.io.invalid> ha scritto:
>>>
>>> I disagree with using `ReferenceCountUtil.safeRelease()` everywhere.
>>> The implementation is throwing any Throwable:
>>>
>>> ```java
>>>
>>> public static void safeRelease(Object msg) {
>>> try {
>>> release(msg);
>>> } catch (Throwable var2) {
>>> logger.warn("Failed to release a message: {}", msg, var2);
>>> }
>>> }
>>> ```
>>>
>>> When `release` throws an exception, it means the logic is wrong. For
>>> example, you have released a ByteBuf whose refcnt is 1 twice. We
>>> should make it clear where fast-fail is not allowed and catch the
>>> exception in these places.
>>>
>>> Thanks,
>>> Yunze
>>>
>>> On Sun, Jan 29, 2023 at 7:57 PM steven lu <lu...@gmail.com> wrote:
>>>>
>>>> [DISCUSS] PIP-244: Refactor ByteBuf release method
>>>> Hello everyone. I hope you guys are all doing well. I would like to start
>>>> the discussion for PIP-244 https://github.com/apache/pulsar/issues/19350,
>>>> Please let me know if you have any concerns or questions.
>>>> ------- Paste original PIP content to help quote ------
>>>>
>>>> ### Motivation
>>>>
>>>> It may throw an exception when release a `ByteBuf` object. so the exception
>>>> in `ByteBuf.release` should be checked.
>>>>
>>>> ### Goal
>>>>
>>>> Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()` in
>>>> all Pulsar's classes.
>>>>
>>>> ### API Changes
>>>>
>>>> _No response_
>>>>
>>>> ### Implementation
>>>>
>>>> 1. Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()`.
>>
Re: [DISCUSS] PIP-244: Refactor ByteBuf release method
Posted by Shiji Lu <lu...@apache.org>.
Good suggestions,
There are three ways to write release in current pulsar and bookkeeper
projects:
1.ByteBuf.release: Call release directly without handling any exceptions;
2.ReferenceCountUtil.release: return processing status;
3. ReferenceCountUtil. safeRelease: without return value, print exception information
I think one specification can be used at present, all of which are released in the way of ReferenceCountUtil.release, replacing these three ways
On 2023/01/30 08:11:39 Enrico Olivelli wrote:
> There is an open discussion on the Apache BooKeeper mailing list about
> this kind of refactoring.
>
> I appreciate this initiative because I see it as a tentative to reduce
> the tech debt.
>
> I think that it is pretty dangerous to do this kind of change.
> Pulsar uses refcounting in a very advanced way and there are many
> subtle cases that deserve careful handling.
>
> I like the motto "Don't break things that aren't broken", this is one
> of the guiding principles while dealing with big open source projects
> and communities as Apache Pulsar
>
>
> Enrico
>
> Il giorno lun 30 gen 2023 alle ore 03:28 Yunze Xu
> <yz...@streamnative.io.invalid> ha scritto:
> >
> > I disagree with using `ReferenceCountUtil.safeRelease()` everywhere.
> > The implementation is throwing any Throwable:
> >
> > ```java
> >
> > public static void safeRelease(Object msg) {
> > try {
> > release(msg);
> > } catch (Throwable var2) {
> > logger.warn("Failed to release a message: {}", msg, var2);
> > }
> > }
> > ```
> >
> > When `release` throws an exception, it means the logic is wrong. For
> > example, you have released a ByteBuf whose refcnt is 1 twice. We
> > should make it clear where fast-fail is not allowed and catch the
> > exception in these places.
> >
> > Thanks,
> > Yunze
> >
> > On Sun, Jan 29, 2023 at 7:57 PM steven lu <lu...@gmail.com> wrote:
> > >
> > > [DISCUSS] PIP-244: Refactor ByteBuf release method
> > > Hello everyone. I hope you guys are all doing well. I would like to start
> > > the discussion for PIP-244 https://github.com/apache/pulsar/issues/19350,
> > > Please let me know if you have any concerns or questions.
> > > ------- Paste original PIP content to help quote ------
> > >
> > > ### Motivation
> > >
> > > It may throw an exception when release a `ByteBuf` object. so the exception
> > > in `ByteBuf.release` should be checked.
> > >
> > > ### Goal
> > >
> > > Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()` in
> > > all Pulsar's classes.
> > >
> > > ### API Changes
> > >
> > > _No response_
> > >
> > > ### Implementation
> > >
> > > 1. Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()`.
>
Re: [DISCUSS] PIP-244: Refactor ByteBuf release method
Posted by Enrico Olivelli <eo...@gmail.com>.
There is an open discussion on the Apache BooKeeper mailing list about
this kind of refactoring.
I appreciate this initiative because I see it as a tentative to reduce
the tech debt.
I think that it is pretty dangerous to do this kind of change.
Pulsar uses refcounting in a very advanced way and there are many
subtle cases that deserve careful handling.
I like the motto "Don't break things that aren't broken", this is one
of the guiding principles while dealing with big open source projects
and communities as Apache Pulsar
Enrico
Il giorno lun 30 gen 2023 alle ore 03:28 Yunze Xu
<yz...@streamnative.io.invalid> ha scritto:
>
> I disagree with using `ReferenceCountUtil.safeRelease()` everywhere.
> The implementation is throwing any Throwable:
>
> ```java
>
> public static void safeRelease(Object msg) {
> try {
> release(msg);
> } catch (Throwable var2) {
> logger.warn("Failed to release a message: {}", msg, var2);
> }
> }
> ```
>
> When `release` throws an exception, it means the logic is wrong. For
> example, you have released a ByteBuf whose refcnt is 1 twice. We
> should make it clear where fast-fail is not allowed and catch the
> exception in these places.
>
> Thanks,
> Yunze
>
> On Sun, Jan 29, 2023 at 7:57 PM steven lu <lu...@gmail.com> wrote:
> >
> > [DISCUSS] PIP-244: Refactor ByteBuf release method
> > Hello everyone. I hope you guys are all doing well. I would like to start
> > the discussion for PIP-244 https://github.com/apache/pulsar/issues/19350,
> > Please let me know if you have any concerns or questions.
> > ------- Paste original PIP content to help quote ------
> >
> > ### Motivation
> >
> > It may throw an exception when release a `ByteBuf` object. so the exception
> > in `ByteBuf.release` should be checked.
> >
> > ### Goal
> >
> > Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()` in
> > all Pulsar's classes.
> >
> > ### API Changes
> >
> > _No response_
> >
> > ### Implementation
> >
> > 1. Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()`.
Re: [DISCUSS] PIP-244: Refactor ByteBuf release method
Posted by Yunze Xu <yz...@streamnative.io.INVALID>.
I disagree with using `ReferenceCountUtil.safeRelease()` everywhere.
The implementation is throwing any Throwable:
```java
public static void safeRelease(Object msg) {
try {
release(msg);
} catch (Throwable var2) {
logger.warn("Failed to release a message: {}", msg, var2);
}
}
```
When `release` throws an exception, it means the logic is wrong. For
example, you have released a ByteBuf whose refcnt is 1 twice. We
should make it clear where fast-fail is not allowed and catch the
exception in these places.
Thanks,
Yunze
On Sun, Jan 29, 2023 at 7:57 PM steven lu <lu...@gmail.com> wrote:
>
> [DISCUSS] PIP-244: Refactor ByteBuf release method
> Hello everyone. I hope you guys are all doing well. I would like to start
> the discussion for PIP-244 https://github.com/apache/pulsar/issues/19350,
> Please let me know if you have any concerns or questions.
> ------- Paste original PIP content to help quote ------
>
> ### Motivation
>
> It may throw an exception when release a `ByteBuf` object. so the exception
> in `ByteBuf.release` should be checked.
>
> ### Goal
>
> Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()` in
> all Pulsar's classes.
>
> ### API Changes
>
> _No response_
>
> ### Implementation
>
> 1. Use `ReferenceCountUtil.safeRelease()` instead of `ByteBuf.release()`.