You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by horizonzy <ho...@apache.org> on 2023/02/17 14:43:08 UTC
The CI tests didn't cover bookkeeper V2 protocol.
In https://github.com/apache/bookkeeper/issues/3751, it report a bug: The
bookkeeper client read operation timeout.
After our research, we found that the bookkeeper client ignore the response
from bookie server. (See https://github.com/apache/bookkeeper/issues/3751
to get the detailed).
The problem is introduced by https://github.com/apache/bookkeeper/pull/3528.
It change the ReadResponse, which is used by V2 Protocol. When config the
netty memory detect policy upper than
ResourceLeakDetector.setLevel(Level.DISABLED), the bookkeeper client will
ignore the response from bookie server, so the read operation timeout.
The https://github.com/apache/bookkeeper/pull/3528 all CI tests succeed and
merged. The Test `Client Tests` shouldn't pass in theory, but it pass.
So we research the root case.
Finnaly, we found probelm which lead the ci pass.
In bookkeeper, we use custom
org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl to
allocate ByteBuf, the memory detect policy config by
`org.apache.bookkeeper.conf.AbstractConfiguration#setAllocatorLeakDetectionPolicy`,
the netty jvm param `-Dio.netty.leakDetection.level` didn't work, it will
be overridden by our custom config.
In our tests, we config `-Dio.netty.leakDetection.level=paranoid`, but it
will be overridden by `disabled`.
So I suggest in our tests, both the clientConf and the serverConfig should
config
`AbstractConfiguration#setAllocatorLeakDetectionPolicy(LeakDetectionPolicy.Paranoid)`.
It can cover more cases.
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by ZhangJian He <sh...@gmail.com>.
+1
Thanks
ZhangJian He
On Tue, 21 Feb 2023 at 10:08, Hang Chen <ch...@apache.org> wrote:
> +1 for Yong's suggestion.
>
> Thanks,
> Hang
>
> Yan Zhao <ho...@apache.org> 于2023年2月20日周一 16:55写道：
> >
> > > According to the original PR's motivation
> > > <https://github.com/apache/bookkeeper/pull/1754>, we wrapped a Netty
> > > allocator
> > > and want to configure something through bookkeeper.
> > > So the user will use our customized allocator and need to obey the
> rules
> > > we introduced. Then the Netty's configuration
> `io.netty.leakDetection.level`
> > > seems useless for us. I think they would never have a chance to take it
> > > differently.
> > >
> > > We defined a new configuration property to configure the
> > > LeakDetectionPolicy.
> > >
> > >
> > >
> > >
> > > *public LeakDetectionPolicy getAllocatorLeakDetectionPolicy() {
> > > return LeakDetectionPolicy
> > > .valueOf(this.getString(ALLOCATOR_LEAK_DETECTION_POLICY,
> > > LeakDetectionPolicy.Paranoid.toString())); }*
> > >
> > > Some users may don't know the `*ALLOCATOR_LEAK_DETECTION_POLICY`*
> > > and only configure* `*io.netty.leakDetection.level` to open the leak
> > > detection.
> > > I would suggest getting both `*ALLOCATOR_LEAK_DETECTION_POLICY`*
> > > and `io.netty.leakDetection.level` from the system property, use the
> highest
> > > policy of it. And then, no matter how you configured it, the detection
> > > policy always worked.
> >
> > Agree.
>
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Hang Chen <ch...@apache.org>.
+1 for Yong's suggestion.
Thanks,
Hang
Yan Zhao <ho...@apache.org> 于2023年2月20日周一 16:55写道：
>
> > According to the original PR's motivation
> > <https://github.com/apache/bookkeeper/pull/1754>, we wrapped a Netty
> > allocator
> > and want to configure something through bookkeeper.
> > So the user will use our customized allocator and need to obey the rules
> > we introduced. Then the Netty's configuration `io.netty.leakDetection.level`
> > seems useless for us. I think they would never have a chance to take it
> > differently.
> >
> > We defined a new configuration property to configure the
> > LeakDetectionPolicy.
> >
> >
> >
> >
> > *public LeakDetectionPolicy getAllocatorLeakDetectionPolicy() {
> > return LeakDetectionPolicy
> > .valueOf(this.getString(ALLOCATOR_LEAK_DETECTION_POLICY,
> > LeakDetectionPolicy.Paranoid.toString())); }*
> >
> > Some users may don't know the `*ALLOCATOR_LEAK_DETECTION_POLICY`*
> > and only configure* `*io.netty.leakDetection.level` to open the leak
> > detection.
> > I would suggest getting both `*ALLOCATOR_LEAK_DETECTION_POLICY`*
> > and `io.netty.leakDetection.level` from the system property, use the highest
> > policy of it. And then, no matter how you configured it, the detection
> > policy always worked.
>
> Agree.
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Yan Zhao <ho...@apache.org>.
> According to the original PR's motivation
> <https://github.com/apache/bookkeeper/pull/1754>, we wrapped a Netty
> allocator
> and want to configure something through bookkeeper.
> So the user will use our customized allocator and need to obey the rules
> we introduced. Then the Netty's configuration `io.netty.leakDetection.level`
> seems useless for us. I think they would never have a chance to take it
> differently.
>
> We defined a new configuration property to configure the
> LeakDetectionPolicy.
>
>
>
>
> *public LeakDetectionPolicy getAllocatorLeakDetectionPolicy() {
> return LeakDetectionPolicy
> .valueOf(this.getString(ALLOCATOR_LEAK_DETECTION_POLICY,
> LeakDetectionPolicy.Paranoid.toString())); }*
>
> Some users may don't know the `*ALLOCATOR_LEAK_DETECTION_POLICY`*
> and only configure* `*io.netty.leakDetection.level` to open the leak
> detection.
> I would suggest getting both `*ALLOCATOR_LEAK_DETECTION_POLICY`*
> and `io.netty.leakDetection.level` from the system property, use the highest
> policy of it. And then, no matter how you configured it, the detection
> policy always worked.
Agree.
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Yong Zhang <zh...@gmail.com>.
Hi Yan,
So I suggest that we introduce a new jvm param
> `preferNettyLeakDetectionPolicy` in bookkeeper, the default value is
> `false`.
>
> If the user config `-DpreferNettyReLeakDetectionPolicy=true`, the
> bookkeeper leak detection policy won't override the netty config. If the ci
> tests, we config `-Dio.netty.leakDetection.level=paranoid
> -DpreferNettyLeakDetectionPolicy=true`, it will use netty leak detection
> level `paranoid`.
>
According to the original PR's motivation
<https://github.com/apache/bookkeeper/pull/1754>, we wrapped a Netty
allocator
and want to configure something through bookkeeper.
So the user will use our customized allocator and need to obey the rules
we introduced. Then the Netty's configuration `io.netty.leakDetection.level`
seems useless for us. I think they would never have a chance to take it
differently.
We defined a new configuration property to configure the
LeakDetectionPolicy.
*public LeakDetectionPolicy getAllocatorLeakDetectionPolicy() {
return LeakDetectionPolicy
.valueOf(this.getString(ALLOCATOR_LEAK_DETECTION_POLICY,
LeakDetectionPolicy.Paranoid.toString())); }*
Some users may don't know the `*ALLOCATOR_LEAK_DETECTION_POLICY`*
and only configure* `*io.netty.leakDetection.level` to open the leak
detection.
I would suggest getting both `*ALLOCATOR_LEAK_DETECTION_POLICY`*
and `io.netty.leakDetection.level` from the system property, use the highest
policy of it. And then, no matter how you configured it, the detection
policy always worked.
Thanks,
Yong
On Mon, 20 Feb 2023 at 10:02, Yan Zhao <ho...@apache.org> wrote:
> > Make sense.
> > There are second level config for the memory detection. The first level
> is netty jvm param `-Dio.netty.leakDetection.level`, the second level is
> bookkeeper config `AbstractConfiguration#setAllocatorLeakDetectionPolicy`.
> >
> > If the second level be config greater than `Disabled`, the second level
> will override the first level.
> > If the second level is `Disabled`, we use the first level config.
>
> The default detection level in netty is `Simple`, the default detection
> level in bookkeeper is `Disabled`. If the user didn't config the bookkeeper
> level detection policy, it will use netty default detection level
> `Simple`, it's a breaking change.
>
> So I suggest that we introduce a new jvm param
> `preferNettyLeakDetectionPolicy` in bookkeeper, the default value is
> `false`.
>
> If the user config `-DpreferNettyReLeakDetectionPolicy=true`, the
> bookkeeper leak detection policy won't override the netty config. If the ci
> tests, we config `-Dio.netty.leakDetection.level=paranoid
> -DpreferNettyLeakDetectionPolicy=true`, it will use netty leak detection
> level `paranoid`.
>
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Yan Zhao <ho...@apache.org>.
> Make sense.
> There are second level config for the memory detection. The first level is netty jvm param `-Dio.netty.leakDetection.level`, the second level is bookkeeper config `AbstractConfiguration#setAllocatorLeakDetectionPolicy`.
>
> If the second level be config greater than `Disabled`, the second level will override the first level.
> If the second level is `Disabled`, we use the first level config.
The default detection level in netty is `Simple`, the default detection level in bookkeeper is `Disabled`. If the user didn't config the bookkeeper level detection policy, it will use netty default detection level `Simple`, it's a breaking change.
So I suggest that we introduce a new jvm param `preferNettyLeakDetectionPolicy` in bookkeeper, the default value is `false`.
If the user config `-DpreferNettyReLeakDetectionPolicy=true`, the bookkeeper leak detection policy won't override the netty config. If the ci tests, we config `-Dio.netty.leakDetection.level=paranoid -DpreferNettyLeakDetectionPolicy=true`, it will use netty leak detection level `paranoid`.
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Yan Zhao <ho...@apache.org>.
> IMO, this is not expected behavior. In our test, both the client and
> server's memory leak detection policy should be gotten from
> `-Dio.netty.leakDetection.level` configuration instead of hard code as
> `LeakDetectionPolicy.Paranoid`
Make sense.
There are second level config for the memory detection. The first level is netty jvm param `-Dio.netty.leakDetection.level`, the second level is bookkeeper config `AbstractConfiguration#setAllocatorOutOfMemoryPolicy`.
If the second level be config greater than `Disabled`, the second level will override the first level.
If the second level is `Disabled`, we use the first level config.
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Hang Chen <ch...@apache.org>.
Hi horizonzy,
Thanks for raising this discussion.
> the netty jvm param `-Dio.netty.leakDetection.level` didn't
work, it will be overridden by our custom config.
> In our tests, we config
`-Dio.netty.leakDetection.level=paranoid`, but it will be overridden
by `disabled`.
IMO, this is not expected behavior. In our test, both the client and
server's memory leak detection policy should be gotten from
`-Dio.netty.leakDetection.level` configuration instead of hard code as
`LeakDetectionPolicy.Paranoid`
Thanks,
Hang
horizonzy <ho...@apache.org> 于2023年2月17日周五 22:43写道：
>
> In https://github.com/apache/bookkeeper/issues/3751, it report a bug: The
> bookkeeper client read operation timeout.
> After our research, we found that the bookkeeper client ignore the response
> from bookie server. (See https://github.com/apache/bookkeeper/issues/3751
> to get the detailed).
>
> The problem is introduced by https://github.com/apache/bookkeeper/pull/3528.
> It change the ReadResponse, which is used by V2 Protocol. When config the
> netty memory detect policy upper than
> ResourceLeakDetector.setLevel(Level.DISABLED), the bookkeeper client will
> ignore the response from bookie server, so the read operation timeout.
>
> The https://github.com/apache/bookkeeper/pull/3528 all CI tests succeed and
> merged. The Test `Client Tests` shouldn't pass in theory, but it pass.
> So we research the root case.
> Finnaly, we found probelm which lead the ci pass.
> In bookkeeper, we use custom
> org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl to
> allocate ByteBuf, the memory detect policy config by
> `org.apache.bookkeeper.conf.AbstractConfiguration#setAllocatorLeakDetectionPolicy`,
> the netty jvm param `-Dio.netty.leakDetection.level` didn't work, it will
> be overridden by our custom config.
> In our tests, we config `-Dio.netty.leakDetection.level=paranoid`, but it
> will be overridden by `disabled`.
>
> So I suggest in our tests, both the clientConf and the serverConfig should
> config
> `AbstractConfiguration#setAllocatorLeakDetectionPolicy(LeakDetectionPolicy.Paranoid)`.
> It can cover more cases.
Re: The CI tests didn't cover bookkeeper V2 protocol.
Posted by Yan Zhao <ho...@apache.org>.
I submit https://github.com/apache/bookkeeper/pull/3794 for it. Please help to view it when you are in inconvenient.