Re: Name of sequence node is not unique

[Kezhu Wang]

Hi,

> Note: the counter used to store the next sequence number is a signed int 
> (4bytes) maintained by the parent node, the counter will overflow when 
> incremented beyond 2147483647 (resulting in a name "-2147483648").

I wrote a test[1] and found that overflow of cversion results in
NODEEXISTS[2]. I think this is better than what the doc[3] says. There
is no wrap-around on the client side.

> In our use case, we create *persistent* *sequential* nodes. We store the 
> sequence id in the client application and use it as a globally unique id.

Given that you have overflowed the cversion, I think you could take a
look at ZOOKEEPER-4332[4]. ZooKeeper does not work well with massive
children when listing. BookKeeper's HierarchicalLedgerManager[5] is a
real world example for this.


> New
> ===
> if (parentCVersion > currentParentCVersion
> *|| parentCVersion == Integer.MIN_VALUE &&
> currentParentCVersion == Integer.MAX_VALUE) *{
> parent.stat.setCversion(parent
> CVersion);
> parent.stat.setPzxid(zxid);
>   }

This breaks "monotonically increasing" and gains no "uniqueness". The
cversion will wrap around again given your cases.

> It would be better to use a longer integer.
>   1.  Increasing to a 64-bit counter certainly solves the problem, but this 
> might require conversion of zk data when the current counter is stored as 
> 32-bit

I support this, but it demands massive work and probably relates to a
long term goal[6].  The "int" fact of "cversion" is exposed both in
API(Stat) and storage(StatPersisted).


[1]: 
https://github.com/kezhuw/zookeeper/commit/755b1168156c28e4fc2813be593ac67514e8bdc7#diff-1af986ce48b5d4bb4b8e51374a70cc6e109a04c70d9f450be3df8f302010341cR59
[2]: 
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L675
[3]: 
https://zookeeper.apache.org/doc/r3.8.1/zookeeperProgrammers.html#Sequence+Nodes+--+Unique+Naming
[4]: https://issues.apache.org/jira/browse/ZOOKEEPER-4332
[5]: 
https://bookkeeper.apache.org/docs/getting-started/concepts/#hierarchical-ledger-manager
[6]: https://issues.apache.org/jira/browse/ZOOKEEPER-102

Kezhu Wang


On Fri, Jun 16, 2023 at 11:33 AM Josef Roehrl
 wrote:
>
> I wanted to add 2 things.
>
>
>   1.  Increasing to a 64-bit counter certainly solves the problem, but this 
> might require conversion of zk data when the current counter is stored as 
> 32-bit
>   2.  A client that relies on a unique version that it uses as a reference 
> outside of zk should verify that a version that it receives does not already 
> exist outside zk. This applies even if 1. is considered, should a zk quorum 
> be reset or lose its data.
>
> Josef Roehrl
> FuseForward | Senior Architect - Professional Services
> [https://fuseforward.atlassian.net/wiki/download/attachments/512327681/image001.png?version=1=1537397840684=1=v2]
> Website
>  | 
> Newsletter
>  | Twitter | 
> LinkedIn
>
> 
> From: Ted Dunning 
> Sent: Thursday, June 15, 2023 6:53 PM
> To: dev@zookeeper.apache.org 
> Subject: Re: Name of sequence node is not unique
>
> The invariant is that the value should be increasing except in failure
> modes. You found a somewhat surprising failure mode.
>
> Please compute how long it would take for a 64-bit counter to overflow if
> incremented a million times per second. (hint, half a million years).
> Remember that zk only does things at less than 100,000 per second
>
> On Thu, Jun 15, 2023, 17:03 Li Wang  wrote:
>
> > Thanks a lot for your inputs, Ted.
> >
> > On Thu, Jun 15, 2023 at 2:52 PM Ted Dunning  wrote:
> >
> > > Breaking a semantic invariant is a dangerous solution here.
> >
> > Totally agree. We should not break a semantic invariant if there is one.
> > What's the semantic invariant here and how ZK is supposed to behave in the
> > overflow case?
> >
> >
> > > It would be better to use a longer integer.
> > >
> > Yes, I thought about this too. Longer integer will overflow too, so the
> > issue of not generating unique numbers will still exist.
> >
> > Best,
> >
> > Li
> >
> > >
> > >
> > >
> > > On Thu, Jun 15, 2023, 13:35 Li Wang  wrote:
> > >
> > > > Thanks for the response, Enrico.
> > > >
> > > > Please see comments below.
> > > >
> > > > On Thu, Jun 15, 2023 at 5:47 AM Enrico Olivelli 
> > > > wrote:
> > > >
> > > > > Li,
> > > > > thanks for reporting your problem.
> > > > >
> > > > > Most likely you have found a bug.
> > > > >
> > > > > I have one question, related to your use case,
> > > > > is the problem that the numbers are not "unique" or that the number

Re: Name of sequence node is not unique

[Josef Roehrl]

I wanted to add 2 things.


  1.  Increasing to a 64-bit counter certainly solves the problem, but this 
might require conversion of zk data when the current counter is stored as 32-bit
  2.  A client that relies on a unique version that it uses as a reference 
outside of zk should verify that a version that it receives does not already 
exist outside zk. This applies even if 1. is considered, should a zk quorum be 
reset or lose its data.

Josef Roehrl​
FuseForward | Senior Architect - Professional Services
[https://fuseforward.atlassian.net/wiki/download/attachments/512327681/image001.png?version=1=1537397840684=1=v2]
Website
 | 
Newsletter
 | Twitter | 
LinkedIn


From: Ted Dunning 
Sent: Thursday, June 15, 2023 6:53 PM
To: dev@zookeeper.apache.org 
Subject: Re: Name of sequence node is not unique

The invariant is that the value should be increasing except in failure
modes. You found a somewhat surprising failure mode.

Please compute how long it would take for a 64-bit counter to overflow if
incremented a million times per second. (hint, half a million years).
Remember that zk only does things at less than 100,000 per second

On Thu, Jun 15, 2023, 17:03 Li Wang  wrote:

> Thanks a lot for your inputs, Ted.
>
> On Thu, Jun 15, 2023 at 2:52 PM Ted Dunning  wrote:
>
> > Breaking a semantic invariant is a dangerous solution here.
>
> Totally agree. We should not break a semantic invariant if there is one.
> What's the semantic invariant here and how ZK is supposed to behave in the
> overflow case?
>
>
> > It would be better to use a longer integer.
> >
> Yes, I thought about this too. Longer integer will overflow too, so the
> issue of not generating unique numbers will still exist.
>
> Best,
>
> Li
>
> >
> >
> >
> > On Thu, Jun 15, 2023, 13:35 Li Wang  wrote:
> >
> > > Thanks for the response, Enrico.
> > >
> > > Please see comments below.
> > >
> > > On Thu, Jun 15, 2023 at 5:47 AM Enrico Olivelli 
> > > wrote:
> > >
> > > > Li,
> > > > thanks for reporting your problem.
> > > >
> > > > Most likely you have found a bug.
> > > >
> > > > I have one question, related to your use case,
> > > > is the problem that the numbers are not "unique" or that the number
> is
> > > > not monotonically increasing ?
> > > >
> > >
> > > Technically speaking, monotonically increasing means either always
> > > increasing or *remaining constant. *With tha*t, * the problem is only
> > > the numbers are not "unique'. In this case, the parent cversion
> > > remains 2147483647
> > > after reaching Integer.MAX_VALUE, not unique any more.
> > >
> > > >
> > > > Do you have 2147483647 concurrent sessions and you found that two
> > > > sessions got the same sequenceId ?
> > > > or are you storing the sequenceId somewhere and you use it as a
> > > > globally unique id, not only among the connected sessions but also
> > > > among all the sessions that are ever connected to the cluster ?
> > > >
> > >
> > > In our use case, we create *persistent* *sequential* nodes. We store
> the
> > > sequence id in the client application and use it as a globally unique
> id.
> > > Currently Zookeeper guarantees the following non-overflow case but not
> > > after overflow.
> > >
> > > 1. Monotonically increasing
> > > 2. Uniqueness
> > > 3. Sequentially increase by 1
> > >
> > > For customers who have an overflow use case and can handle the sequence
> > > number cycling in a circular fashion, how about having a simple patch
> > > to support it and handle the overflow case better?  The change is
> adding
> > a
> > > condition to allow the wraparound when it flows to negative. We can
> also
> > > have a property to control whether to add the additional condition if
> > > needed.
> > >
> > > New
> > > ===
> > > if (parentCVersion > currentParentCVersion
> > > *|| parentCVersion == Integer.MIN_VALUE &&
> > > currentParentCVersion == Integer.MAX_VALUE) *{
> > > parent.stat.setCversion(parentCVersion);
> > > parent.stat.setPzxid(zxid);
> > >   }
> > >
> > > Current
> > > =
> > > if (parentCVersion > parent.stat.getCversion()) {
> > > parent.stat.setCversion(parentCVersion);
> > > parent.stat.setPzxid(zxid);
> > > }
> > >
> > > Please let me know what you think. Any input or feedback would be
> > > appreciated.
> > >
> > > Thanks,
> > >
> > > Li
> > >
> > >
> > > > Enrico
> > > >
> > > > Il giorno ven 9 giu 2023 alle ore 21:10 Li Wang 
> ha
> > > > scritto:
> > > > >
> > > > > Hello,
> > > > >
> > > > > We are running 3.7.1 in production and running into an "issue" that
> > the
> > > > 

Re: Name of sequence node is not unique

[Ted Dunning]

The invariant is that the value should be increasing except in failure
modes. You found a somewhat surprising failure mode.

Please compute how long it would take for a 64-bit counter to overflow if
incremented a million times per second. (hint, half a million years).
Remember that zk only does things at less than 100,000 per second

On Thu, Jun 15, 2023, 17:03 Li Wang  wrote:

> Thanks a lot for your inputs, Ted.
>
> On Thu, Jun 15, 2023 at 2:52 PM Ted Dunning  wrote:
>
> > Breaking a semantic invariant is a dangerous solution here.
>
> Totally agree. We should not break a semantic invariant if there is one.
> What's the semantic invariant here and how ZK is supposed to behave in the
> overflow case?
>
>
> > It would be better to use a longer integer.
> >
> Yes, I thought about this too. Longer integer will overflow too, so the
> issue of not generating unique numbers will still exist.
>
> Best,
>
> Li
>
> >
> >
> >
> > On Thu, Jun 15, 2023, 13:35 Li Wang  wrote:
> >
> > > Thanks for the response, Enrico.
> > >
> > > Please see comments below.
> > >
> > > On Thu, Jun 15, 2023 at 5:47 AM Enrico Olivelli 
> > > wrote:
> > >
> > > > Li,
> > > > thanks for reporting your problem.
> > > >
> > > > Most likely you have found a bug.
> > > >
> > > > I have one question, related to your use case,
> > > > is the problem that the numbers are not "unique" or that the number
> is
> > > > not monotonically increasing ?
> > > >
> > >
> > > Technically speaking, monotonically increasing means either always
> > > increasing or *remaining constant. *With tha*t, * the problem is only
> > > the numbers are not "unique'. In this case, the parent cversion
> > > remains 2147483647
> > > after reaching Integer.MAX_VALUE, not unique any more.
> > >
> > > >
> > > > Do you have 2147483647 concurrent sessions and you found that two
> > > > sessions got the same sequenceId ?
> > > > or are you storing the sequenceId somewhere and you use it as a
> > > > globally unique id, not only among the connected sessions but also
> > > > among all the sessions that are ever connected to the cluster ?
> > > >
> > >
> > > In our use case, we create *persistent* *sequential* nodes. We store
> the
> > > sequence id in the client application and use it as a globally unique
> id.
> > > Currently Zookeeper guarantees the following non-overflow case but not
> > > after overflow.
> > >
> > > 1. Monotonically increasing
> > > 2. Uniqueness
> > > 3. Sequentially increase by 1
> > >
> > > For customers who have an overflow use case and can handle the sequence
> > > number cycling in a circular fashion, how about having a simple patch
> > > to support it and handle the overflow case better?  The change is
> adding
> > a
> > > condition to allow the wraparound when it flows to negative. We can
> also
> > > have a property to control whether to add the additional condition if
> > > needed.
> > >
> > > New
> > > ===
> > > if (parentCVersion > currentParentCVersion
> > > *|| parentCVersion == Integer.MIN_VALUE &&
> > > currentParentCVersion == Integer.MAX_VALUE) *{
> > > parent.stat.setCversion(parentCVersion);
> > > parent.stat.setPzxid(zxid);
> > >   }
> > >
> > > Current
> > > =
> > > if (parentCVersion > parent.stat.getCversion()) {
> > > parent.stat.setCversion(parentCVersion);
> > > parent.stat.setPzxid(zxid);
> > > }
> > >
> > > Please let me know what you think. Any input or feedback would be
> > > appreciated.
> > >
> > > Thanks,
> > >
> > > Li
> > >
> > >
> > > > Enrico
> > > >
> > > > Il giorno ven 9 giu 2023 alle ore 21:10 Li Wang 
> ha
> > > > scritto:
> > > > >
> > > > > Hello,
> > > > >
> > > > > We are running 3.7.1 in production and running into an "issue" that
> > the
> > > > > names of sequence nodes are not unique after the counter hits the
> max
> > > int
> > > > > (i.e 2147483647) and overflows.  I would like to start a thread to
> > > > discuss
> > > > > the following
> > > > >
> > > > > 1. Is this a bug or "expected" behavior?
> > > > > 2. Is ZK supposed to support the overflow scenario and need to make
> > > sure
> > > > > the name is unique when overflow happens?
> > > > >
> > > > > The name is not unique after hitting the max int value because of
> we
> > > > > have the following in zk  code base:
> > > > >
> > > > > 1.  The cversion of parent znode is used to build the child name in
> > > > > PrepRequestProcessor
> > > > >
> > > > > int parentCVersion = parentRecord.stat.getCversion();
> > > > > if (createMode.isSequential()) {
> > > > > path = path + String.format(Locale.ENGLISH, "%010d",
> > > > > parentCVersion);
> > > > > }
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/
> > > > >
> java/org/apache/zookeeper/server/PrepRequestProcessor.java#L668-L671
> > > > >
> > > > >
> > > > > 2. The parent znode is read from 

Re: Name of sequence node is not unique

[Li Wang]

Thanks a lot for your inputs, Ted.

On Thu, Jun 15, 2023 at 2:52 PM Ted Dunning  wrote:

> Breaking a semantic invariant is a dangerous solution here.

Totally agree. We should not break a semantic invariant if there is one.
What's the semantic invariant here and how ZK is supposed to behave in the
overflow case?


> It would be better to use a longer integer.
>
Yes, I thought about this too. Longer integer will overflow too, so the
issue of not generating unique numbers will still exist.

Best,

Li

>
>
>
> On Thu, Jun 15, 2023, 13:35 Li Wang  wrote:
>
> > Thanks for the response, Enrico.
> >
> > Please see comments below.
> >
> > On Thu, Jun 15, 2023 at 5:47 AM Enrico Olivelli 
> > wrote:
> >
> > > Li,
> > > thanks for reporting your problem.
> > >
> > > Most likely you have found a bug.
> > >
> > > I have one question, related to your use case,
> > > is the problem that the numbers are not "unique" or that the number is
> > > not monotonically increasing ?
> > >
> >
> > Technically speaking, monotonically increasing means either always
> > increasing or *remaining constant. *With tha*t, * the problem is only
> > the numbers are not "unique'. In this case, the parent cversion
> > remains 2147483647
> > after reaching Integer.MAX_VALUE, not unique any more.
> >
> > >
> > > Do you have 2147483647 concurrent sessions and you found that two
> > > sessions got the same sequenceId ?
> > > or are you storing the sequenceId somewhere and you use it as a
> > > globally unique id, not only among the connected sessions but also
> > > among all the sessions that are ever connected to the cluster ?
> > >
> >
> > In our use case, we create *persistent* *sequential* nodes. We store the
> > sequence id in the client application and use it as a globally unique id.
> > Currently Zookeeper guarantees the following non-overflow case but not
> > after overflow.
> >
> > 1. Monotonically increasing
> > 2. Uniqueness
> > 3. Sequentially increase by 1
> >
> > For customers who have an overflow use case and can handle the sequence
> > number cycling in a circular fashion, how about having a simple patch
> > to support it and handle the overflow case better?  The change is adding
> a
> > condition to allow the wraparound when it flows to negative. We can also
> > have a property to control whether to add the additional condition if
> > needed.
> >
> > New
> > ===
> > if (parentCVersion > currentParentCVersion
> > *|| parentCVersion == Integer.MIN_VALUE &&
> > currentParentCVersion == Integer.MAX_VALUE) *{
> > parent.stat.setCversion(parentCVersion);
> > parent.stat.setPzxid(zxid);
> >   }
> >
> > Current
> > =
> > if (parentCVersion > parent.stat.getCversion()) {
> > parent.stat.setCversion(parentCVersion);
> > parent.stat.setPzxid(zxid);
> > }
> >
> > Please let me know what you think. Any input or feedback would be
> > appreciated.
> >
> > Thanks,
> >
> > Li
> >
> >
> > > Enrico
> > >
> > > Il giorno ven 9 giu 2023 alle ore 21:10 Li Wang  ha
> > > scritto:
> > > >
> > > > Hello,
> > > >
> > > > We are running 3.7.1 in production and running into an "issue" that
> the
> > > > names of sequence nodes are not unique after the counter hits the max
> > int
> > > > (i.e 2147483647) and overflows.  I would like to start a thread to
> > > discuss
> > > > the following
> > > >
> > > > 1. Is this a bug or "expected" behavior?
> > > > 2. Is ZK supposed to support the overflow scenario and need to make
> > sure
> > > > the name is unique when overflow happens?
> > > >
> > > > The name is not unique after hitting the max int value because of we
> > > > have the following in zk  code base:
> > > >
> > > > 1.  The cversion of parent znode is used to build the child name in
> > > > PrepRequestProcessor
> > > >
> > > > int parentCVersion = parentRecord.stat.getCversion();
> > > > if (createMode.isSequential()) {
> > > > path = path + String.format(Locale.ENGLISH, "%010d",
> > > > parentCVersion);
> > > > }
> > > >
> > > >
> > > >
> > >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/
> > > > java/org/apache/zookeeper/server/PrepRequestProcessor.java#L668-L671
> > > >
> > > >
> > > > 2. The parent znode is read from either zks.outstandingChangesForPath
> > map
> > > > or zk database/datatree.
> > > >
> > > >lastChange = zks.outstandingChangesForPath.get(path);
> > > > if (lastChange == null) {
> > > > DataNode n = zks.getZKDatabase().getNode(path);
> > > >
> > > >
> > > >
> > >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L168-L170
> > > >
> > > >
> > > >
> > > > 3. The cversion of the parent node in outstandingChangesForPath map
> is
> > > > always updated  but not in zk database as we added the following code
> > in
> > > 3.6
> > > >
> > > >   

Re: Name of sequence node is not unique

[Ted Dunning]

Breaking a semantic invariant is a dangerous solution here. It would be
better to use a longer integer.



On Thu, Jun 15, 2023, 13:35 Li Wang  wrote:

> Thanks for the response, Enrico.
>
> Please see comments below.
>
> On Thu, Jun 15, 2023 at 5:47 AM Enrico Olivelli 
> wrote:
>
> > Li,
> > thanks for reporting your problem.
> >
> > Most likely you have found a bug.
> >
> > I have one question, related to your use case,
> > is the problem that the numbers are not "unique" or that the number is
> > not monotonically increasing ?
> >
>
> Technically speaking, monotonically increasing means either always
> increasing or *remaining constant. *With tha*t, * the problem is only
> the numbers are not "unique'. In this case, the parent cversion
> remains 2147483647
> after reaching Integer.MAX_VALUE, not unique any more.
>
> >
> > Do you have 2147483647 concurrent sessions and you found that two
> > sessions got the same sequenceId ?
> > or are you storing the sequenceId somewhere and you use it as a
> > globally unique id, not only among the connected sessions but also
> > among all the sessions that are ever connected to the cluster ?
> >
>
> In our use case, we create *persistent* *sequential* nodes. We store the
> sequence id in the client application and use it as a globally unique id.
> Currently Zookeeper guarantees the following non-overflow case but not
> after overflow.
>
> 1. Monotonically increasing
> 2. Uniqueness
> 3. Sequentially increase by 1
>
> For customers who have an overflow use case and can handle the sequence
> number cycling in a circular fashion, how about having a simple patch
> to support it and handle the overflow case better?  The change is adding a
> condition to allow the wraparound when it flows to negative. We can also
> have a property to control whether to add the additional condition if
> needed.
>
> New
> ===
> if (parentCVersion > currentParentCVersion
> *|| parentCVersion == Integer.MIN_VALUE &&
> currentParentCVersion == Integer.MAX_VALUE) *{
> parent.stat.setCversion(parentCVersion);
> parent.stat.setPzxid(zxid);
>   }
>
> Current
> =
> if (parentCVersion > parent.stat.getCversion()) {
> parent.stat.setCversion(parentCVersion);
> parent.stat.setPzxid(zxid);
> }
>
> Please let me know what you think. Any input or feedback would be
> appreciated.
>
> Thanks,
>
> Li
>
>
> > Enrico
> >
> > Il giorno ven 9 giu 2023 alle ore 21:10 Li Wang  ha
> > scritto:
> > >
> > > Hello,
> > >
> > > We are running 3.7.1 in production and running into an "issue" that the
> > > names of sequence nodes are not unique after the counter hits the max
> int
> > > (i.e 2147483647) and overflows.  I would like to start a thread to
> > discuss
> > > the following
> > >
> > > 1. Is this a bug or "expected" behavior?
> > > 2. Is ZK supposed to support the overflow scenario and need to make
> sure
> > > the name is unique when overflow happens?
> > >
> > > The name is not unique after hitting the max int value because of we
> > > have the following in zk  code base:
> > >
> > > 1.  The cversion of parent znode is used to build the child name in
> > > PrepRequestProcessor
> > >
> > > int parentCVersion = parentRecord.stat.getCversion();
> > > if (createMode.isSequential()) {
> > > path = path + String.format(Locale.ENGLISH, "%010d",
> > > parentCVersion);
> > > }
> > >
> > >
> > >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/
> > > java/org/apache/zookeeper/server/PrepRequestProcessor.java#L668-L671
> > >
> > >
> > > 2. The parent znode is read from either zks.outstandingChangesForPath
> map
> > > or zk database/datatree.
> > >
> > >lastChange = zks.outstandingChangesForPath.get(path);
> > > if (lastChange == null) {
> > > DataNode n = zks.getZKDatabase().getNode(path);
> > >
> > >
> > >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L168-L170
> > >
> > >
> > >
> > > 3. The cversion of the parent node in outstandingChangesForPath map is
> > > always updated  but not in zk database as we added the following code
> in
> > 3.6
> > >
> > > if (parentCVersion > parent.stat.getCversion()) {
> > > parent.stat.setCversion(parentCVersion);
> > > parent.stat.setPzxid(zxid);
> > > }
> > >
> > >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L477-L480
> > >
> > > https://issues.apache.org/jira/browse/ZOOKEEPER-3249
> > >
> > >
> > > When overflow happens, the new parentCversion is changed to
> -2147483648.
> > > It's updated in the outstandingChangesForPath map. It's not updated in
> > > DataTree and the value stays as 2147483647  because -2147483648 is less
> > > than 2147483647, 

[jira] [Created] (ZOOKEEPER-4705) Restrict GitHub merge butten to allow squash commit only

[Andor Molnar (Jira)]

Andor Molnar created ZOOKEEPER-4705:
---

 Summary: Restrict GitHub merge butten to allow squash commit only
 Key: ZOOKEEPER-4705
 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-4705
 Project: ZooKeeper
  Issue Type: Improvement
  Components: build-infrastructure
Affects Versions: 3.8.1, 3.7.1
Reporter: Andor Molnar
Assignee: Andor Molnar
 Fix For: 3.9.0, 3.7.2, 3.8.2


Based on 
[https://cwiki.apache.org/confluence/pages/viewpage.action?spaceKey=INFRA=git+-+.asf.yaml+features#Git.asf.yamlfeatures-Mergebuttons]

 

Add limitation to .asf.yaml.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Re: Name of sequence node is not unique

[Li Wang]

Thanks for the response, Enrico.

Please see comments below.

On Thu, Jun 15, 2023 at 5:47 AM Enrico Olivelli  wrote:

> Li,
> thanks for reporting your problem.
>
> Most likely you have found a bug.
>
> I have one question, related to your use case,
> is the problem that the numbers are not "unique" or that the number is
> not monotonically increasing ?
>

Technically speaking, monotonically increasing means either always
increasing or *remaining constant. *With tha*t, * the problem is only
the numbers are not "unique'. In this case, the parent cversion
remains 2147483647
after reaching Integer.MAX_VALUE, not unique any more.

>
> Do you have 2147483647 concurrent sessions and you found that two
> sessions got the same sequenceId ?
> or are you storing the sequenceId somewhere and you use it as a
> globally unique id, not only among the connected sessions but also
> among all the sessions that are ever connected to the cluster ?
>

In our use case, we create *persistent* *sequential* nodes. We store the
sequence id in the client application and use it as a globally unique id.
Currently Zookeeper guarantees the following non-overflow case but not
after overflow.

1. Monotonically increasing
2. Uniqueness
3. Sequentially increase by 1

For customers who have an overflow use case and can handle the sequence
number cycling in a circular fashion, how about having a simple patch
to support it and handle the overflow case better?  The change is adding a
condition to allow the wraparound when it flows to negative. We can also
have a property to control whether to add the additional condition if
needed.

New
===
if (parentCVersion > currentParentCVersion
*|| parentCVersion == Integer.MIN_VALUE &&
currentParentCVersion == Integer.MAX_VALUE) *{
parent.stat.setCversion(parentCVersion);
parent.stat.setPzxid(zxid);
  }

Current
=
if (parentCVersion > parent.stat.getCversion()) {
parent.stat.setCversion(parentCVersion);
parent.stat.setPzxid(zxid);
}

Please let me know what you think. Any input or feedback would be
appreciated.

Thanks,

Li


> Enrico
>
> Il giorno ven 9 giu 2023 alle ore 21:10 Li Wang  ha
> scritto:
> >
> > Hello,
> >
> > We are running 3.7.1 in production and running into an "issue" that the
> > names of sequence nodes are not unique after the counter hits the max int
> > (i.e 2147483647) and overflows.  I would like to start a thread to
> discuss
> > the following
> >
> > 1. Is this a bug or "expected" behavior?
> > 2. Is ZK supposed to support the overflow scenario and need to make sure
> > the name is unique when overflow happens?
> >
> > The name is not unique after hitting the max int value because of we
> > have the following in zk  code base:
> >
> > 1.  The cversion of parent znode is used to build the child name in
> > PrepRequestProcessor
> >
> > int parentCVersion = parentRecord.stat.getCversion();
> > if (createMode.isSequential()) {
> > path = path + String.format(Locale.ENGLISH, "%010d",
> > parentCVersion);
> > }
> >
> >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/
> > java/org/apache/zookeeper/server/PrepRequestProcessor.java#L668-L671
> >
> >
> > 2. The parent znode is read from either zks.outstandingChangesForPath map
> > or zk database/datatree.
> >
> >lastChange = zks.outstandingChangesForPath.get(path);
> > if (lastChange == null) {
> > DataNode n = zks.getZKDatabase().getNode(path);
> >
> >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L168-L170
> >
> >
> >
> > 3. The cversion of the parent node in outstandingChangesForPath map is
> > always updated  but not in zk database as we added the following code in
> 3.6
> >
> > if (parentCVersion > parent.stat.getCversion()) {
> > parent.stat.setCversion(parentCVersion);
> > parent.stat.setPzxid(zxid);
> > }
> >
> >
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L477-L480
> >
> > https://issues.apache.org/jira/browse/ZOOKEEPER-3249
> >
> >
> > When overflow happens, the new parentCversion is changed to -2147483648.
> > It's updated in the outstandingChangesForPath map. It's not updated in
> > DataTree and the value stays as 2147483647  because -2147483648 is less
> > than 2147483647, so the cVerson is inconsistent in  ZK.
> >
> > Due to the inconsistent cVersion, when the next request comes in after
> > overflow, the sequence number is non-deterministic and not unique
> depending
> > on where the parent node is read from.  It can be 2147483647 if the
> > parent node is read from DataTree or -2147483648,  -2147483647 and so on
> if
> > it's from the outstandingChangesForPath map.
> >
> > We have the 

Re: Netty native libraries in ZooKeeper

[Andor Molnar]

Yeah, it works fine with the hardcoded classifier.
I'll make this Pulsar approach in the patch.

Andor



On Thu, 2023-06-15 at 19:50 +0200, Enrico Olivelli wrote:
> I may be wrong but Epoll is only a Linux thing probably.
> 
> You don't have it on Mac or Windows
> 
> Enrico
> 
> Il Gio 15 Giu 2023, 19:49 Andor Molnar  ha scritto:
> 
> > Interesting that with only the BOM included and the dependencies
> > without the classifier, Netty doesn't load the native epoll
> > selector,
> > but loads the native SSL library.
> > 
> > I'm confused.
> > 
> > In the Pulsar example the sub-project dependency has a hardcoded
> > classifier:
> > 
> > 
> >   io.netty
> >   netty-transport-native-epoll
> >   linux-x86_64
> > 
> > 
> > Andor
> > 
> > 
> > 
> > On Thu, 2023-06-15 at 14:31 +0200, Enrico Olivelli wrote:
> > > I think that the best way currently is to add these dependencies:
> > > 
> > > Import the Netty BOM in the main pom.xml
> > > 
> > >   io.netty
> > >   netty-bom
> > >   ${netty.version}
> > >   pom
> > >   import
> > > 
> > > 
> > > 
> > > declare netty dependencies without setting the version and the
> > > classifier
> > > 
> > >   io.netty
> > >   netty-tcnative-boringssl-static
> > > 
> > > 
> > > This is the way we are doing it in Pulsar
> > > 
> > https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pom.xml#L647
> > https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pulsar-common/pom.xml#L146
> > > This way Maven should bundle all the native libraries for all the
> > > supported platforms
> > > 
> > > 
> > > 
> > > Enrico
> > > 
> > > Il giorno gio 15 giu 2023 alle ore 12:50 Andor Molnar
> > >  ha scritto:
> > > > Hi,
> > > > 
> > > > I've come across the following when working on the support of
> > > > native
> > > > SSL libraries. Currently ZooKeeper supports loading the native
> > > > epoll-
> > > > based event loop of Netty, but a build profile which would
> > > > download
> > > > the
> > > > required dependencies is not shipped with our product.
> > > > 
> > > > This is perfectly okay since the feature of using native
> > > > libraries
> > > > is
> > > > not a build-time requirement, but in this case the user has to
> > > > download
> > > > the required and appropriate versions of Netty jars and put
> > > > them on
> > > > the
> > > > classpath.
> > > > 
> > > > Shall we add a Maven build profile to ease this process?
> > > > 
> > > > 
> > > >   netty-native
> > > >   
> > > > fedora > > > sifi
> > > > erWi
> > > > thLikes>
> > > >   
> > > >   
> > > > 
> > > >   
> > > > io.netty
> > > > netty-tcnative-boringssl-
> > > > static
> > > > ${netty-tcnative.version}
> > > > ${os.detected.classifier}
> > > >   
> > > >   
> > > > io.netty
> > > > netty-transport-native-epoll
> > > > ${netty.version}
> > > > ${os.detected.classifier}
> > > >   
> > > > 
> > > >   
> > > > 
> > > > 
> > > > What do you think?
> > > > 
> > > > Andor
> > > > 
> > > > 
> > > > 

Re: Netty native libraries in ZooKeeper

[Enrico Olivelli]

I may be wrong but Epoll is only a Linux thing probably.

You don't have it on Mac or Windows

Enrico

Il Gio 15 Giu 2023, 19:49 Andor Molnar  ha scritto:

> Interesting that with only the BOM included and the dependencies
> without the classifier, Netty doesn't load the native epoll selector,
> but loads the native SSL library.
>
> I'm confused.
>
> In the Pulsar example the sub-project dependency has a hardcoded
> classifier:
>
> 
>   io.netty
>   netty-transport-native-epoll
>   linux-x86_64
> 
>
> Andor
>
>
>
> On Thu, 2023-06-15 at 14:31 +0200, Enrico Olivelli wrote:
> > I think that the best way currently is to add these dependencies:
> >
> > Import the Netty BOM in the main pom.xml
> > 
> >   io.netty
> >   netty-bom
> >   ${netty.version}
> >   pom
> >   import
> > 
> >
> >
> > declare netty dependencies without setting the version and the
> > classifier
> > 
> >   io.netty
> >   netty-tcnative-boringssl-static
> > 
> >
> > This is the way we are doing it in Pulsar
> >
> https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pom.xml#L647
> >
> https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pulsar-common/pom.xml#L146
> >
> > This way Maven should bundle all the native libraries for all the
> > supported platforms
> >
> >
> >
> > Enrico
> >
> > Il giorno gio 15 giu 2023 alle ore 12:50 Andor Molnar
> >  ha scritto:
> > > Hi,
> > >
> > > I've come across the following when working on the support of
> > > native
> > > SSL libraries. Currently ZooKeeper supports loading the native
> > > epoll-
> > > based event loop of Netty, but a build profile which would download
> > > the
> > > required dependencies is not shipped with our product.
> > >
> > > This is perfectly okay since the feature of using native libraries
> > > is
> > > not a build-time requirement, but in this case the user has to
> > > download
> > > the required and appropriate versions of Netty jars and put them on
> > > the
> > > classpath.
> > >
> > > Shall we add a Maven build profile to ease this process?
> > >
> > > 
> > >   netty-native
> > >   
> > > fedora > > erWi
> > > thLikes>
> > >   
> > >   
> > > 
> > >   
> > > io.netty
> > > netty-tcnative-boringssl-static
> > > ${netty-tcnative.version}
> > > ${os.detected.classifier}
> > >   
> > >   
> > > io.netty
> > > netty-transport-native-epoll
> > > ${netty.version}
> > > ${os.detected.classifier}
> > >   
> > > 
> > >   
> > > 
> > >
> > > What do you think?
> > >
> > > Andor
> > >
> > >
> > >
>
>

Re: Netty native libraries in ZooKeeper

[Andor Molnar]

Interesting that with only the BOM included and the dependencies
without the classifier, Netty doesn't load the native epoll selector,
but loads the native SSL library.

I'm confused.

In the Pulsar example the sub-project dependency has a hardcoded
classifier: 


  io.netty
  netty-transport-native-epoll
  linux-x86_64


Andor



On Thu, 2023-06-15 at 14:31 +0200, Enrico Olivelli wrote:
> I think that the best way currently is to add these dependencies:
> 
> Import the Netty BOM in the main pom.xml
> 
>   io.netty
>   netty-bom
>   ${netty.version}
>   pom
>   import
> 
> 
> 
> declare netty dependencies without setting the version and the
> classifier
> 
>   io.netty
>   netty-tcnative-boringssl-static
> 
> 
> This is the way we are doing it in Pulsar
> https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pom.xml#L647
> https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pulsar-common/pom.xml#L146
> 
> This way Maven should bundle all the native libraries for all the
> supported platforms
> 
> 
> 
> Enrico
> 
> Il giorno gio 15 giu 2023 alle ore 12:50 Andor Molnar
>  ha scritto:
> > Hi,
> > 
> > I've come across the following when working on the support of
> > native
> > SSL libraries. Currently ZooKeeper supports loading the native
> > epoll-
> > based event loop of Netty, but a build profile which would download
> > the
> > required dependencies is not shipped with our product.
> > 
> > This is perfectly okay since the feature of using native libraries
> > is
> > not a build-time requirement, but in this case the user has to
> > download
> > the required and appropriate versions of Netty jars and put them on
> > the
> > classpath.
> > 
> > Shall we add a Maven build profile to ease this process?
> > 
> > 
> >   netty-native
> >   
> > fedora > erWi
> > thLikes>
> >   
> >   
> > 
> >   
> > io.netty
> > netty-tcnative-boringssl-static
> > ${netty-tcnative.version}
> > ${os.detected.classifier}
> >   
> >   
> > io.netty
> > netty-transport-native-epoll
> > ${netty.version}
> > ${os.detected.classifier}
> >   
> > 
> >   
> > 
> > 
> > What do you think?
> > 
> > Andor
> > 
> > 
> > 

Re: FIPS: removing ZKTrustManager

[Andor Molnar]

Thanks Enrico, we've made a mistake though: discussed that fips-mode
will be enabled by default on master branch and disabled by default on
branch-3.8.

Let me create a separate pull request for that.

Andor



On Thu, 2023-06-15 at 14:39 +0200, Enrico Olivelli wrote:
> Il giorno mer 14 giu 2023 alle ore 13:43 Andor Molnar
>  ha scritto:
> > PR has been created with the proposed resolution:
> > 
> > https://github.com/apache/zookeeper/pull/2008
> 
> Committed to master and branch-3.8
> 
> Thank you
> Enrico
> 
> > Please review.
> > 
> > Thanks,
> > Andor
> > 
> > 
> > 
> > On Sat, 2023-06-10 at 11:25 +0200, Andor Molnar wrote:
> > > "we use this method dozens of other places in the code"
> > > 
> > > Checked. Mostly logging and output formatting like 4lws, etc.
> > > 
> > > 
> > > 
> > > On Sat, 2023-06-10 at 11:18 +0200, Andor Molnar wrote:
> > > > First, I've created a pull request for ZOOKEEPER-3860:
> > > > 
> > > > https://github.com/apache/zookeeper/pull/2005
> > > > 
> > > > To improve the logging in ZKTrustManager without altering the
> > > > behaviour. The patch also contains a change in
> > > > NetUtils.formatInetAddr() which, I believe, should use the
> > > > hostname
> > > > when creating textual representation of an InetAddress. I'm not
> > > > 100%
> > > > sure about this, because while it certainly helps in TLS cases
> > > > to
> > > > avoid
> > > > unnecessary reverse DNS lookups, we use this method dozens of
> > > > other
> > > > places in the code. Unit tests are passsing.
> > > > 
> > > > ZOOKEEPER-4268
> > > > 
> > > > It's about reverse lookups in the client code, but I haven't
> > > > found
> > > > the
> > > > reported behaviour on latest master, so just closed the ticket.
> > > > 
> > > > Andor
> > > > 
> > > > 
> > > > 
> > > > On Fri, 2023-06-09 at 18:29 +0200, Szalay-Bekő Máté wrote:
> > > > > yeah, I remember these tickets, thanks for picking them up!
> > > > > I agree and like the solution you proposed, in general in the
> > > > > long
> > > > > term it
> > > > > is good not to use a custom trust manager, but rely on the
> > > > > standard
> > > > > one.
> > > > > 
> > > > > Máté
> > > > > 
> > > > > 
> > > > > On Fri, Jun 9, 2023 at 2:08 PM Enrico Olivelli <
> > > > > eolive...@gmail.com
> > > > > wrote:
> > > > > 
> > > > > > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> > > > > >  ha scritto:
> > > > > > > I'd like to backport this to the 3.8 branch too.
> > > > > > > 
> > > > > > > Let's say I'll add new "zookeeper.fips-mode" parameter
> > > > > > > which
> > > > > > > will
> > > > > > > be
> > > > > > > "false" by default in 3.8 and "true" for 3.9.0.
> > > > > > 
> > > > > > I am +1
> > > > > > ZK 3.9 will take time to be adopted and this is an
> > > > > > important
> > > > > > security
> > > > > > related topic
> > > > > > 
> > > > > > Enrico
> > > > > > 
> > > > > > > Thoughts?
> > > > > > > 
> > > > > > > Andor
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, 2023-06-09 at 13:55 +0200, Enrico Olivelli wrote:
> > > > > > > > I think that switching to
> > > > > > > > sslParameters.setEndpointIdentificationAlgorithm("HTTPS
> > > > > > > > ");
> > > > > > > > is
> > > > > > > > a
> > > > > > > > good
> > > > > > > > option.
> > > > > > > > The less tweaks we have about Security code the better.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > It would be great to see this in 3.9.0.
> > > > > > > > 
> > > > > > > > Enrico
> > > > > > > > 
> > > > > > > > Il giorno ven 9 giu 2023 alle ore 13:42 Andor Molnar
> > > > > > > >  ha scritto:
> > > > > > > > > Hi zk folks,
> > > > > > > > > 
> > > > > > > > > Problem(s)
> > > > > > > > > ==
> > > > > > > > > 
> > > > > > > > > One problem that we're having with a custom Trust
> > > > > > > > > Manager
> > > > > > > > > in
> > > > > > > > > ZK is
> > > > > > > > > that
> > > > > > > > > FIPS doesn't allow that:
> > > > > > > > > 
> > > > > > > > > https://issues.apache.org/jira/browse/ZOOKEEPER-4393
> > > > > > > > > 
> > > > > > > > > In FIPS mode the only allowed TrustManager in the JDK
> > > > > > > > > is
> > > > > > > > > X509TrustManagerImpl which is the default
> > > > > > > > > implementation.
> > > > > > > > > The
> > > > > > > > > class
> > > > > > > > > is
> > > > > > > > > final, so extending it is not an option
> > > > > > > > > unfortunately.
> > > > > > > > > 
> > > > > > > > > The intention behind implementing a custom trust
> > > > > > > > > manager
> > > > > > > > > in
> > > > > > > > > ZK was,
> > > > > > > > > I
> > > > > > > > > believe, the need for server and client-side hostname
> > > > > > > > > verification.
> > > > > > > > > Hostname verification officially is not part of the
> > > > > > > > > SSL/TLS
> > > > > > > > > protocol,
> > > > > > > > > it's the responsibility of an upper level protocol
> > > > > > > > > like
> > > > > > > > > HTTPS.
> > > > > > > > > 
> > > > > > > > > Hacking hostname verification in the SSL handshake is
> > > > > > > > > nice
> > > > > > > > 

Re: Name of sequence node is not unique

[Enrico Olivelli]

Li,
thanks for reporting your problem.

Most likely you have found a bug.

I have one question, related to your use case,
is the problem that the numbers are not "unique" or that the number is
not monotonically increasing ?

Do you have 2147483647 concurrent sessions and you found that two
sessions got the same sequenceId ?
or are you storing the sequenceId somewhere and you use it as a
globally unique id, not only among the connected sessions but also
among all the sessions that are ever connected to the cluster ?

Enrico

Il giorno ven 9 giu 2023 alle ore 21:10 Li Wang  ha scritto:
>
> Hello,
>
> We are running 3.7.1 in production and running into an "issue" that the
> names of sequence nodes are not unique after the counter hits the max int
> (i.e 2147483647) and overflows.  I would like to start a thread to discuss
> the following
>
> 1. Is this a bug or "expected" behavior?
> 2. Is ZK supposed to support the overflow scenario and need to make sure
> the name is unique when overflow happens?
>
> The name is not unique after hitting the max int value because of we
> have the following in zk  code base:
>
> 1.  The cversion of parent znode is used to build the child name in
> PrepRequestProcessor
>
> int parentCVersion = parentRecord.stat.getCversion();
> if (createMode.isSequential()) {
> path = path + String.format(Locale.ENGLISH, "%010d",
> parentCVersion);
> }
>
>
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/
> java/org/apache/zookeeper/server/PrepRequestProcessor.java#L668-L671
>
>
> 2. The parent znode is read from either zks.outstandingChangesForPath map
> or zk database/datatree.
>
>lastChange = zks.outstandingChangesForPath.get(path);
> if (lastChange == null) {
> DataNode n = zks.getZKDatabase().getNode(path);
>
>
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java#L168-L170
>
>
>
> 3. The cversion of the parent node in outstandingChangesForPath map is
> always updated  but not in zk database as we added the following code in 3.6
>
> if (parentCVersion > parent.stat.getCversion()) {
> parent.stat.setCversion(parentCVersion);
> parent.stat.setPzxid(zxid);
> }
>
> https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java#L477-L480
>
> https://issues.apache.org/jira/browse/ZOOKEEPER-3249
>
>
> When overflow happens, the new parentCversion is changed to -2147483648.
> It's updated in the outstandingChangesForPath map. It's not updated in
> DataTree and the value stays as 2147483647  because -2147483648 is less
> than 2147483647, so the cVerson is inconsistent in  ZK.
>
> Due to the inconsistent cVersion, when the next request comes in after
> overflow, the sequence number is non-deterministic and not unique depending
> on where the parent node is read from.  It can be 2147483647 if the
> parent node is read from DataTree or -2147483648,  -2147483647 and so on if
> it's from the outstandingChangesForPath map.
>
> We have the following doc about unique naming but no info on  "expected"
> behavior after overflow.
>
> Sequence Nodes -- Unique Naming
>
>
> When creating a znode you can also request that ZooKeeper append a
> monotonically increasing counter to the end of path. This counter is unique
> to the parent znode. The counter has a format of %010d -- that is 10 digits
> with 0 (zero) padding (the counter is formatted in this way to simplify
> sorting), i.e. "01". See Queue Recipe for an example use of this
> feature. Note: the counter used to store the next sequence number is a
> signed int (4bytes) maintained by the parent node, the counter will
> overflow when incremented beyond 2147483647 (resulting in a name
> "-2147483648").
>
>
>
> Please let me know if you have any comments or inputs.
>
>
> Thanks,
>
>
> Li

Re: Review request for ZOOKEEPER-4471 and ZOOKEEPER-4472

[Enrico Olivelli]

Your patches have been merged to master branch and they will be
shipped with 3.9.0.

Thank you
Enrico

Il giorno sab 10 giu 2023 alle ore 18:13 Kezhu Wang 
ha scritto:
>
> Hi all and committers,
>
> [ZOOKEEPER-4471][1] reported that `AddWatchMode.PERSISTENT` could be
> partially removed by `WatcherType.Data` or `WatcherType.Children`.
>
> [ZOOKEEPER-4472][2] proposed to add `WatcherType.Persistent` and
> `WatcherType.PersistentRecursive` to remove `AddWatchMode.PERSISTENT`
> and `AddWatchMode.PERSISTENT_RECURSIVE` respectively.
>
> [ZOOKEEPER-4466][3] rescues us from conflict among different watch
> types on the same path. So, clients can watch whatever paths in
> whatever modes. They will not ruin each other.
>
> On the other hand, [ZOOKEEPER-4472][2] complements the removing part
> of [ZOOKEEPER-4466][3]. If a client wants to remove a persistent
> watch, it will issue a `removeWatches` `WatcherType.Persistent`. This
> will not affect any other watcher types. Currently, clients have to
> resort to `WatcherType.Any` to remove them. This could potentially
> affect other ongoing watcher types.
>
> Ideally, ZOOKEEPER-4472 is independent of ZOOKEEPER-4471, but it would
> be hard to ship only ZOOKEEPER-4472 without a fix to ZOOKEEPER-4471.
> That will require particular attention to avoid trigger paths for
> ZOOKEEPER-4471. That is why I delayed it until now.
>
> I saw discussions about [cut release for 3.9][4]. I really hope we can
> merge [pr#1998][5] for ZOOKEEPER-4471 and [pr#2006][6] for
> ZOOKEEPER-4472 in 3.9.0. It would be impossible for us to merge
> ZOOKEEPER-4472 to patch versions of 3.9 series as it touches both
> server logic and api side while ZOOKEEPER-4466 is shipped into 3.9. It
> looks weird if we support different watcher types one same path in
> watching but not all of them in removing. That is why I hope we can
> ship them along with ZOOKEEPER-4466 in 3.9.0.
>
> I plan to reply with a short message about this possibility in the
> release discussion thread to ref back this mail. Hope it won't bore
> you in there or here.
>
> Anyway, please take your time to review pr#1998 and pr#2006 no matter
> whether they will be included in 3.9.0.
>
> Last, if you are curious how ZOOKEEPER-4472 could affect a real
> program. You can take a look at [try_remove_watcher][7] and
> [dispatch_path_event][8] in [zookeeper-rust-client][9]. The
> `try_remove_watcher` ignores dropping of persistent watches if there
> are other watches remaining. But this will leak persistent watches on
> the server side. `dispatch_path_event` handles this, it will issue
> `removeWatches` with `WatcherType.Any` if it receives events to a path
> with no watchers. For no persistent watches, the client will issue
> corresponding `WatcherType`s if there are more watchers with them. But
> it can't do the same for persistent watches due to leak of
> `WatcherType.Persistent` and `WatcherType.PersistentRecursive`. Hope
> this convinces you of  ZOOKEEPER-4472.
>
> [1]: https://issues.apache.org/jira/browse/ZOOKEEPER-4471
> [2]: https://issues.apache.org/jira/browse/ZOOKEEPER-4472
> [3]: https://issues.apache.org/jira/browse/ZOOKEEPER-4466
> [4]: https://lists.apache.org/thread/dgnt1xnlf5n9tzt7m4otbodg2qdx3fz4
> [5]: https://github.com/apache/zookeeper/pull/1998
> [6]: https://github.com/apache/zookeeper/pull/2006
> [7]: 
> https://github.com/kezhuw/zookeeper-client-rust/blob/7466e867fb1b229d6b6ffb230e2e682c49ff494e/src/session/watch.rs#L368-L388
> [8]: 
> https://github.com/kezhuw/zookeeper-client-rust/blob/7466e867fb1b229d6b6ffb230e2e682c49ff494e/src/session/watch.rs#L341-L366
> [9]: https://github.com/kezhuw/zookeeper-client-rust
>
> Best,
> Kezhu Wang

Re: planning release 3.8.2

[Enrico Olivelli]

Thanks for volounteering

Il giorno mer 14 giu 2023 alle ore 14:31 Szalay-Bekő Máté
 ha scritto:
>
> Hello ZooKeepers,
>
> release 3.8.1 happened this January and I volunteered to do 3.8.2 soon,
> maybe in June or early July. Let me know if someone would also like to do
> it, I am happy to hand it over! :)
>
> Of course we need to do all the 3pp CVE / vulnerability fixes first.
> And Andor already started a thread about a FIPS TLS improvement which we
> should wait for.
>
> Does anyone know about any other open ticket we should wait for in 3.8.2?

Nothing that I am aware of.

Best regards
Enrico

>
> Best regards,
> Máté

Re: FIPS: removing ZKTrustManager

[Enrico Olivelli]

Il giorno mer 14 giu 2023 alle ore 13:43 Andor Molnar
 ha scritto:
>
> PR has been created with the proposed resolution:
>
> https://github.com/apache/zookeeper/pull/2008


Committed to master and branch-3.8

Thank you
Enrico

> Please review.
>
> Thanks,
> Andor
>
>
>
> On Sat, 2023-06-10 at 11:25 +0200, Andor Molnar wrote:
> > "we use this method dozens of other places in the code"
> >
> > Checked. Mostly logging and output formatting like 4lws, etc.
> >
> >
> >
> > On Sat, 2023-06-10 at 11:18 +0200, Andor Molnar wrote:
> > > First, I've created a pull request for ZOOKEEPER-3860:
> > >
> > > https://github.com/apache/zookeeper/pull/2005
> > >
> > > To improve the logging in ZKTrustManager without altering the
> > > behaviour. The patch also contains a change in
> > > NetUtils.formatInetAddr() which, I believe, should use the hostname
> > > when creating textual representation of an InetAddress. I'm not
> > > 100%
> > > sure about this, because while it certainly helps in TLS cases to
> > > avoid
> > > unnecessary reverse DNS lookups, we use this method dozens of other
> > > places in the code. Unit tests are passsing.
> > >
> > > ZOOKEEPER-4268
> > >
> > > It's about reverse lookups in the client code, but I haven't found
> > > the
> > > reported behaviour on latest master, so just closed the ticket.
> > >
> > > Andor
> > >
> > >
> > >
> > > On Fri, 2023-06-09 at 18:29 +0200, Szalay-Bekő Máté wrote:
> > > > yeah, I remember these tickets, thanks for picking them up!
> > > > I agree and like the solution you proposed, in general in the
> > > > long
> > > > term it
> > > > is good not to use a custom trust manager, but rely on the
> > > > standard
> > > > one.
> > > >
> > > > Máté
> > > >
> > > >
> > > > On Fri, Jun 9, 2023 at 2:08 PM Enrico Olivelli <
> > > > eolive...@gmail.com
> > > > wrote:
> > > >
> > > > > Il giorno ven 9 giu 2023 alle ore 14:07 Andor Molnar
> > > > >  ha scritto:
> > > > > > I'd like to backport this to the 3.8 branch too.
> > > > > >
> > > > > > Let's say I'll add new "zookeeper.fips-mode" parameter which
> > > > > > will
> > > > > > be
> > > > > > "false" by default in 3.8 and "true" for 3.9.0.
> > > > >
> > > > > I am +1
> > > > > ZK 3.9 will take time to be adopted and this is an important
> > > > > security
> > > > > related topic
> > > > >
> > > > > Enrico
> > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > Andor
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Fri, 2023-06-09 at 13:55 +0200, Enrico Olivelli wrote:
> > > > > > > I think that switching to
> > > > > > > sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
> > > > > > > is
> > > > > > > a
> > > > > > > good
> > > > > > > option.
> > > > > > > The less tweaks we have about Security code the better.
> > > > > > >
> > > > > > >
> > > > > > > It would be great to see this in 3.9.0.
> > > > > > >
> > > > > > > Enrico
> > > > > > >
> > > > > > > Il giorno ven 9 giu 2023 alle ore 13:42 Andor Molnar
> > > > > > >  ha scritto:
> > > > > > > > Hi zk folks,
> > > > > > > >
> > > > > > > > Problem(s)
> > > > > > > > ==
> > > > > > > >
> > > > > > > > One problem that we're having with a custom Trust Manager
> > > > > > > > in
> > > > > > > > ZK is
> > > > > > > > that
> > > > > > > > FIPS doesn't allow that:
> > > > > > > >
> > > > > > > > https://issues.apache.org/jira/browse/ZOOKEEPER-4393
> > > > > > > >
> > > > > > > > In FIPS mode the only allowed TrustManager in the JDK is
> > > > > > > > X509TrustManagerImpl which is the default implementation.
> > > > > > > > The
> > > > > > > > class
> > > > > > > > is
> > > > > > > > final, so extending it is not an option unfortunately.
> > > > > > > >
> > > > > > > > The intention behind implementing a custom trust manager
> > > > > > > > in
> > > > > > > > ZK was,
> > > > > > > > I
> > > > > > > > believe, the need for server and client-side hostname
> > > > > > > > verification.
> > > > > > > > Hostname verification officially is not part of the
> > > > > > > > SSL/TLS
> > > > > > > > protocol,
> > > > > > > > it's the responsibility of an upper level protocol like
> > > > > > > > HTTPS.
> > > > > > > >
> > > > > > > > Hacking hostname verification in the SSL handshake is
> > > > > > > > nice
> > > > > > > > and was
> > > > > > > > working fine so far, but unfortunately breaks the FIPS
> > > > > > > > standard.
> > > > > > > >
> > > > > > > > Another annoying issue with ZKTrustManager is the need
> > > > > > > > for
> > > > > > > > reverse
> > > > > > > > DNS
> > > > > > > > lookup. This is usually needed when the hostname of the
> > > > > > > > certificate
> > > > > > > > provider is not known at the time of handshake. For
> > > > > > > > instance,
> > > > > > > > when
> > > > > > > > somebody connects the client via IP address, which is
> > > > > > > > generally not
> > > > > > > > recommended when TLS is active in the client-server
> > > > > > > > protocol.
> > > > > > > >
> > > > > > > > The bigger problem I've found is in the leader election:
> > > > > 

Re: JDK 21 is in Rampdown / The importance of testing with Early-Access Builds

[Enrico Olivelli]

This is our Jenkins job
https://ci-hadoop.apache.org/view/ZooKeeper/job/ZooKeeper-Java-EA/

The latest build passed.

We are using openjdk-21-ea+21 on Linux.

it seems that it is not the very latest EA (is it +26 ?)

>From the output of the jenkins job run:

[ZooKeeper-Java-EA] $ /home/jenkins/tools/maven/apache-maven-3.9.2/bin/mvn -v
Apache Maven 3.9.2 (c9616018c7a021c1c39be70fb2843d6f5f9b8a1c)
Maven home: /home/jenkins/tools/maven/apache-maven-3.9.2
Java version: 21-ea, vendor: Oracle Corporation, runtime:
/usr/local/asfpackages/java/openjdk-21-ea+21
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "4.15.0-206-generic", arch: "amd64", family: "unix"

Enrico

Il giorno mer 14 giu 2023 alle ore 10:44 David Delabassee
 ha scritto:
>
> Welcome to the OpenJDK Quality Outreach June update.
>
> JDK 21 has entered Rampdown Phase One (RDP1) [1], which means that the 
> main-line has been forked into a dedicated JDK 21 stabilization repository. 
> At this point, the overall JDK 21 feature set is frozen. Any changes pushed 
> to the main line are now bound for JDK 22. The stabilization repository is 
> open for select bug fixes and, with approval, late low-risk enhancements per 
> the JDK Release Process [2]. And in contrast to past practice, most 
> stabilization changes will be integrated via backports from the main-line 
> repository [1].
>
> The coming weeks are critical to identify and resolve as many issues as 
> possible, i.e. before JDK 21 enters the Release Candidates phase in August. 
> We need to make sure those few weeks are leveraged to test both existing code 
> running on top of JDK 21 and new JDK 21 features. The heads-up below 
> illustrates the importance and the benefits of doing such tests.
>
> [1] https://mail.openjdk.org/pipermail/jdk-dev/2023-June/007911.html
> [2] https://openjdk.org/jeps/3#Integrating-fixes-and-enhancements
>
>
> ## Heads-up: On the Importance of Doing Tests With OpenJDK Early-Access Builds
>
> The following is a recent example that demonstrates the benefits of testing 
> an existing codebase using the OpenJDK early-access builds.
>
> Last month, we published a heads-up focused on Sequenced Collections [3] as 
> they could potentially introduce some incompatibilities.
> The Eclipse Collections (EC) team did their homework and sure enough, EC was 
> impacted as it was now throwing compilation errors with JDK 21 early-access 
> builds. The EC team was able to quickly fix those compilation errors, i.e., 
> it was mainly about adding overriding default methods. But once those 
> compilation errors were fixed, and this is where it gets interesting, another 
> issue surfaced. This time, the problem was related to LinkedHashMap 
> serialization. After some investigation, the EC team identified that second 
> issue as JDK one and a JBS ticket was opened. That issue was then confirmed 
> as a JDK regression and was promptly fixed in OpenJDK main-line, i.e., JDK 
> 22. The fix was then backported into the JDK 21 stabilization repository. 
> This EC pull request [4] provides additional details.
> In this case, the JDK fix was easy but it is nevertheless the kind of issues 
> that could have easily fallen through the crack if the EC team wasn’t 
> pro-actively testing with OpenJDK early-access builds. The EC issue would 
> have then surfaced after the JDK 21 General Availability... and who knows 
> when the JDK LinkedHashMap serialization regression would have been fixed?
> TL; DR; Testing an existing codebase with OpenJDK early-access builds is a 
> win-win situation. It helps the project itself, Eclipse Collections in this 
> case, as it enables developers to identify issues in their own codebase 
> before that new JDK version is Generally Available. It helps the JDK too as 
> any JDK issue detected early enough in the development cycle gives the 
> OpenJDK engineers a chance to address it before the General Availability of 
> that new JDK version. And last but not least, having a robust JDK is also a 
> win for the Java community at large.
>
> And thanks to the Eclipse Collections team and especially to Don Raab for 
> helping to make the Java platform more robust!
>
> [3] https://inside.java/2023/05/12/quality-heads-up/
> [4] https://github.com/eclipse/eclipse-collections/pull/1461
>
>
> ## JDK 21 Early-Access Builds
>
> JDK 21 Early-Access builds 26 are now available [5], and are provided under 
> the GNU General Public License v2, with the Classpath Exception. The Release 
> Notes are available here [6] and the javadocs here [7].
>
> ### JEPs integrated into JDK 21:
> - 430: String Templates (Preview)
> - 431: Sequenced Collections
> - 439: Generational ZGC
> - 440: Record Patterns
> - 441: Pattern Matching for switch
> - 442: Foreign Function & Memory API (3rd Preview)
> - 443: Unnamed Patterns and Variables (Preview)
> - 444: Virtual Threads
> - 445: Unnamed Classes and Instance Main Methods (Preview)
> - 446: Scoped Values (Preview)
> - 448: Vector 

Re: Netty native libraries in ZooKeeper

[Enrico Olivelli]

I think that the best way currently is to add these dependencies:

Import the Netty BOM in the main pom.xml

  io.netty
  netty-bom
  ${netty.version}
  pom
  import



declare netty dependencies without setting the version and the classifier

  io.netty
  netty-tcnative-boringssl-static


This is the way we are doing it in Pulsar
https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pom.xml#L647
https://github.com/apache/pulsar/blob/d7f355881b2b1eebf2be6ea262c202660d684fb7/pulsar-common/pom.xml#L146

This way Maven should bundle all the native libraries for all the
supported platforms



Enrico

Il giorno gio 15 giu 2023 alle ore 12:50 Andor Molnar
 ha scritto:
>
> Hi,
>
> I've come across the following when working on the support of native
> SSL libraries. Currently ZooKeeper supports loading the native epoll-
> based event loop of Netty, but a build profile which would download the
> required dependencies is not shipped with our product.
>
> This is perfectly okay since the feature of using native libraries is
> not a build-time requirement, but in this case the user has to download
> the required and appropriate versions of Netty jars and put them on the
> classpath.
>
> Shall we add a Maven build profile to ease this process?
>
> 
>   netty-native
>   
> fedora thLikes>
>   
>   
> 
>   
> io.netty
> netty-tcnative-boringssl-static
> ${netty-tcnative.version}
> ${os.detected.classifier}
>   
>   
> io.netty
> netty-transport-native-epoll
> ${netty.version}
> ${os.detected.classifier}
>   
> 
>   
> 
>
> What do you think?
>
> Andor
>
>
>

Re: ZooKeeper release 3.9.0

[Enrico Olivelli]

Il giorno gio 15 giu 2023 alle ore 13:58 Andor Molnar
 ha scritto:
>
> Hi folks,
>
> There're 64 open tickets which has fixVersion = 3.9.0
> I'll remove the fixVersion from all of them except the ones that we
> marked as release blockers.

I agree.
It is useless to keep the "fixVersion" on an issue that is not a
blocker for a release.
it adds only noise

Enrico

>
> Currently:
>
> - ZOOKEEPER-4393 Problem to connect to zookeeper in FIPS mode
> - ZOOKEEPER-4622 Add Netty-TcNative OpenSSL Support
> - ZOOKEEPER-4655 Communicate the Zxid that triggered a WatchEvent to
> fire
>
> Please let me know if you would like to add anything to this list.
>
> Regards,
> Andor
>
>
>

ZooKeeper release 3.9.0

[Andor Molnar]

Hi folks,

There're 64 open tickets which has fixVersion = 3.9.0
I'll remove the fixVersion from all of them except the ones that we
marked as release blockers.

Currently:

- ZOOKEEPER-4393 Problem to connect to zookeeper in FIPS mode
- ZOOKEEPER-4622 Add Netty-TcNative OpenSSL Support 
- ZOOKEEPER-4655 Communicate the Zxid that triggered a WatchEvent to
fire

Please let me know if you would like to add anything to this list.

Regards,
Andor

Netty native libraries in ZooKeeper

[Andor Molnar]

Hi,

I've come across the following when working on the support of native
SSL libraries. Currently ZooKeeper supports loading the native epoll-
based event loop of Netty, but a build profile which would download the
required dependencies is not shipped with our product.

This is perfectly okay since the feature of using native libraries is
not a build-time requirement, but in this case the user has to download
the required and appropriate versions of Netty jars and put them on the
classpath.

Shall we add a Maven build profile to ease this process?


  netty-native
  
fedora
  
  

  
io.netty
netty-tcnative-boringssl-static
${netty-tcnative.version}
${os.detected.classifier}
  
  
io.netty
netty-transport-native-epoll
${netty.version}
${os.detected.classifier}
  

  


What do you think?

Andor