Re: [DISCUSS] Is it better for individual OpCode.check to get UNIMPLEMENTED

[Zili Chen <ti...@apache.org>]

Hi Kezhu,

Thanks for bringing up this topic!

The main issue seems to be maintenance considerations instead of user-facing concerns.

UPDATE - 

I found that we can simply edit `boolean validpacket = Request.isValid(si.type);` to return false in OpCode.check. Then it's a good trade-off to apply your proposal. +1.

ORIGIN - 

If so, I wonder how we can distinguish individual requests from those from TXN?

Also, IIRC client API doesn't support sending individual requests with OpCode.check at all. Perhaps we can make it an assertion so that nothing should be changed.

Best,
tison.

On 2023/10/17 03:57:26 Kezhu Wang wrote:
> Hi devs,
> 
> It is a long time issue that OpCode.check can cause client
> disconnection and server unusable or even database corruption. It has
> been reported twice in ZOOKEEPER-2623[1] and ZOOKEEPER-4680[2]. Both
> are reported by third-party client authors.
> 
> I opened pr-1988[3](merged now) to make OpCode.check a pure read
> operation and return nothing to match OpResult.CheckResult in
> MultiResponse. But I am rethinking this when investigating
> ZOOKEEPER-4750[4] and wonder whether UNIMPLEMENTED is more appropriate
> for reasons:
> 1. OpCode.check was designed to function inside OpCode.multi but not
> individually.
> 2. We have OpCode.exists, it covers the OpCode.check behavior
> pr-1988[3] introduced.
> 3. It could confuse us when inspecting logs and metrics if we record
> operation statistics somewhere. This probably introduces a long term
> maintenance burden.
> 
> So I am here to seek a consensus on this. I prefer to UNIMPLEMENTED
> even pr-1988[3] has been merged.
> 
> @anmolnar might want to backport this to 3.8 and 3.9 series. I am
> supportive of backport. So I think we probably should align before
> backporting.
> 
> Any thoughts ?
> 
> [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-2623
> [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4680
> [3]: https://github.com/apache/zookeeper/pull/1988
> [4]: https://issues.apache.org/jira/browse/ZOOKEEPER-4750
> 

Re: [DISCUSS] Is it better for individual OpCode.check to get UNIMPLEMENTED

[Kezhu Wang <ke...@gmail.com>]

Hi tison,

Yes, it should be as simple as you pointed out. I have open pr-2104[1]
and pr-2105[2] for this.

[1]: https://github.com/apache/zookeeper/pull/2104
[2]: https://github.com/apache/zookeeper/pull/2105

Best,
Kezhu Wang


On Fri, Nov 10, 2023 at 8:27 PM Zili Chen <ti...@apache.org> wrote:
>
> Hi Kezhu,
>
> Thanks for bringing up this topic!
>
> The main issue seems to be maintenance considerations instead of user-facing concerns.
>
> UPDATE -
>
> I found that we can simply edit `boolean validpacket = Request.isValid(si.type);` to return false in OpCode.check. Then it's a good trade-off to apply your proposal. +1.
>
> ORIGIN -
>
> If so, I wonder how we can distinguish individual requests from those from TXN?
>
> Also, IIRC client API doesn't support sending individual requests with OpCode.check at all. Perhaps we can make it an assertion so that nothing should be changed.
>
> Best,
> tison.
>
> On 2023/10/17 03:57:26 Kezhu Wang wrote:
> > Hi devs,
> >
> > It is a long time issue that OpCode.check can cause client
> > disconnection and server unusable or even database corruption. It has
> > been reported twice in ZOOKEEPER-2623[1] and ZOOKEEPER-4680[2]. Both
> > are reported by third-party client authors.
> >
> > I opened pr-1988[3](merged now) to make OpCode.check a pure read
> > operation and return nothing to match OpResult.CheckResult in
> > MultiResponse. But I am rethinking this when investigating
> > ZOOKEEPER-4750[4] and wonder whether UNIMPLEMENTED is more appropriate
> > for reasons:
> > 1. OpCode.check was designed to function inside OpCode.multi but not
> > individually.
> > 2. We have OpCode.exists, it covers the OpCode.check behavior
> > pr-1988[3] introduced.
> > 3. It could confuse us when inspecting logs and metrics if we record
> > operation statistics somewhere. This probably introduces a long term
> > maintenance burden.
> >
> > So I am here to seek a consensus on this. I prefer to UNIMPLEMENTED
> > even pr-1988[3] has been merged.
> >
> > @anmolnar might want to backport this to 3.8 and 3.9 series. I am
> > supportive of backport. So I think we probably should align before
> > backporting.
> >
> > Any thoughts ?
> >
> > [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-2623
> > [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4680
> > [3]: https://github.com/apache/zookeeper/pull/1988
> > [4]: https://issues.apache.org/jira/browse/ZOOKEEPER-4750
> >