You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Randgalt <gi...@git.apache.org> on 2017/09/22 22:20:28 UTC
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
GitHub user Randgalt opened a pull request:
https://github.com/apache/zookeeper/pull/377
[ZOOKEEPER-2901] TTL Nodes don't work with Server IDs > 127
There was a major oversight when TTL nodes were implemented. The session ID generator for each server is seeded with the configured Server ID in the high byte. TTL Nodes were using the highest bit to denote a TTL node when used in the ephemeral owner. This meant that Server IDs > 127 that created ephemeral nodes would have those nodes always considered TTL nodes (with the TTL being essentially a random number).
This PR fixes the issue by disabling TTL Nodes by default. They must now be enabled in zoo.cfg. When TTL Nodes are enabled, the max Server ID changes from 255 to 254. This allows the high byte of a session ID stored in the ephemeral owner to use 0xFF to denote a TTL node.
About this change:
- The doc has been updated to note that TTL nodes are disabled by default and must be enabled via config. Also, the docs explains that when TTL nodes are enabled the max Server ID becomes 254
- The TTL implementation has been updated to use 0xFF in the high byte of the ephemeralOwner to denote a TTL node. This decreases the max TTL by an insignificant amount
- PrepRequestProcessor now throws `KeeperException.UnimplementedException` when an attempt to create a TTL node is made but the server has not been configured to enable TTL Nodes.
- QuorumPeer throws a `RuntimeException` if TTL Nodes are enabled but the Server ID > 254
- Tests have been added to validate all of this
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Randgalt/zookeeper ZOOKEEPER-2901
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/377.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #377
----
commit 21fa7716133e903c8d43c1003012837f980cb044
Author: randgalt <jo...@jordanzimmerman.com>
Date: 2017-09-22T22:10:30Z
There was a major oversight when TTL nodes were implemented. The session ID generator for each server is seeded with the configured
Server ID in the high byte. TTL Nodes were using the highest bit to denote a TTL node when used in the ephemeral owner. This meant
that Server IDs > 127 that created ephemeral nodes would have those nodes always considered TTL nodes (with the TTL being essentially
a random number).
This PR fixes the issue by disabling TTL Nodes by default. They must now be enabled in zoo.cfg. When TTL Nodes are enabled, the max
Server ID changes from 255 to 254. This allows the high byte of a session ID stored in the ephemeral owner to use 0xFF to denote
a TTL node.
----
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
IMPORTANT NOTE: TTL Nodes created in 3.5.3 will revert to EPHEMERAL with this change. We need to discuss the impact of this and consider workarounds, etc.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r169863794
--- Diff: src/java/main/org/apache/zookeeper/server/OldEphemeralType.java ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+/**
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-2901
+ *
+ * version 3.5.3 introduced bugs associated with how TTL nodes were implemented. version 3.5.4
+ * fixes the problems but makes TTL nodes created in 3.5.3 invalid. OldEphemeralType is a copy
+ * of the old - bad - implementation that is provided as a workaround. {@link EphemeralType#TTL_3_5_3_EMULATION_PROPERTY}
+ * can be used to emulate support of the badly specified TTL nodes.
+ */
+public enum OldEphemeralType {
+ /**
+ * Not ephemeral
+ */
+ VOID,
+ /**
+ * Standard, pre-3.5.x EPHEMERAL
+ */
+ NORMAL,
+ /**
+ * Container node
+ */
+ CONTAINER,
+ /**
+ * TTL node
+ */
+ TTL;
+
+ public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
+ public static final long MAX_TTL = 0x0fffffffffffffffL;
+ public static final long TTL_MASK = 0x8000000000000000L;
+
+ public static OldEphemeralType get(long ephemeralOwner) {
--- End diff --
Do think it has value to keep this "old" type in the codebase?
For backward compatibility reason I don't think it makes sense on a non-stable branch.
Its contents only used from the "new" type and tests.
I'd rather put the logic which is still needed in the "new" type and get rid of this one completely keeping EphemeralType nice and clean.
What do you think?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r141200554
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -125,6 +125,8 @@
private final ZooKeeperServerListener listener;
private ZooKeeperServerShutdownHandler zkShutdownHandler;
+ private volatile boolean defaultTtlNodesEnabled = true;
--- End diff --
for the sake of clarity, why can't this be false?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r156809696
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -949,14 +949,15 @@ server.3=zoo3:2888:3888</programlisting>
</varlistentry>
<varlistentry>
- <term>ttlNodesEnabled</term>
+ <term>zookeeper.extendedTypesEnabled</term>
<listitem>
- <para>(No Java system property)</para>
+ <para>(Java system property only: <emphasis
--- End diff --
This is a bit confusing given QuorumPeerConfig line 326
System.setProperty("zookeeper." + key, value);
See the doc for forceSync for an example of a better way to set this up. (forceSync works this same way)
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt @phunt I think this PR is in a pretty good shape, we should finalize it. Any thoughts or outstanding concerns?
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt I had to resolve some conflicts in order to compare this to current master. Posted the updated branch here: https://github.com/phunt/zookeeper/commits/ZOOKEEPER-2901 Can you review this and update this patch when you have a chance (I'm still reviewing/testing the updates at that link)
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
There are 4 tests with this line of code and it's failing because the logger is not there (NullPointerException). Any ideas?
```
Layout layout =
Logger.getRootLogger().getAppender("CONSOLE").getLayout();
```
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
FYI - I did some local ad-hoc testing with server IDs 254/255 and the config value true/false in zoo.cfg and everything works as expected.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
Attn: @phunt - I just pushed two changes:
- Better docs and new reserved bits in the `EphemeralType` enum.
- Better implementation of the testable `serverId` in ZooKeeperServer.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r161147063
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -40,19 +39,40 @@
TTL;
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_TTL = 0x00ffffffffffffffL;
+ public static final long TTL_MASK = 0xff00000000000000L;
+ public static final long MAX_TTL_SERVER_ID = 0xfe; // 254
+
+ public static final String EXTENDED_TYPES_ENABLED_PROPERTY = "zookeeper.extendedTypesEnabled";
+
+ public static boolean extendedEphemeralTypesEnabled() {
+ return Boolean.getBoolean(EXTENDED_TYPES_ENABLED_PROPERTY);
+ }
public static EphemeralType get(long ephemeralOwner) {
+ if ( extendedEphemeralTypesEnabled() ) {
+ if ((ephemeralOwner & TTL_MASK) == TTL_MASK) {
+ return TTL;
+ }
+ }
if (ephemeralOwner == CONTAINER_EPHEMERAL_OWNER) {
return CONTAINER;
}
- if (ephemeralOwner < 0) {
- return TTL;
- }
return (ephemeralOwner == 0) ? VOID : NORMAL;
}
+ public static void validateServerId(long serverId) {
--- End diff --
Maybe we should not start the server if the server id is conflict with TTL feature, we can do this in QuorumPeerConfig.checkValidity.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
Hi @Randgalt - this looks like a reasonable change to me however it's not working. I used the sample zoo.cfg file as my config and started via bin/zkServer.sh - however I can still create a ttl based node even though "ttlNodesEnabled=false" in the zoo.cfg. I recommend you add a test for this.
Can you take a look?
Also I'd appreciate if you could rebase against current apache master. I did this locally manually, however I'm not sure how the submission script is going to manage it (there were conflicts because you had merged multiple times, including from third party repos (abe)). I can probably figure it out, but if you're working to address this issue perhaps you can fix this too. thx.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r156821178
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -949,14 +949,15 @@ server.3=zoo3:2888:3888</programlisting>
</varlistentry>
<varlistentry>
- <term>ttlNodesEnabled</term>
+ <term>zookeeper.extendedTypesEnabled</term>
<listitem>
- <para>(No Java system property)</para>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.extendedTypesEnabled</emphasis>)</para>
- <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Set to "true" to enable
- the creation of <ulink url="zookeeperProgrammers.html#TTL+Nodes">TTL Nodes</ulink>.
- They are disabled by default. IMPORTANT: when TTL Nodes are enabled server IDs must
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Define to "true" to enable
+ extended features such as the creation of <ulink url="zookeeperProgrammers.html#TTL+Nodes">TTL Nodes</ulink>.
--- End diff --
obv update this to reflect.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r186427922
--- Diff: src/java/main/org/apache/zookeeper/server/OldEphemeralType.java ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+/**
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-2901
+ *
+ * version 3.5.3 introduced bugs associated with how TTL nodes were implemented. version 3.5.4
+ * fixes the problems but makes TTL nodes created in 3.5.3 invalid. OldEphemeralType is a copy
+ * of the old - bad - implementation that is provided as a workaround. {@link EphemeralType#TTL_3_5_3_EMULATION_PROPERTY}
+ * can be used to emulate support of the badly specified TTL nodes.
+ */
+public enum OldEphemeralType {
+ /**
+ * Not ephemeral
+ */
+ VOID,
+ /**
+ * Standard, pre-3.5.x EPHEMERAL
+ */
+ NORMAL,
+ /**
+ * Container node
+ */
+ CONTAINER,
+ /**
+ * TTL node
+ */
+ TTL;
+
+ public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
+ public static final long MAX_TTL = 0x0fffffffffffffffL;
+ public static final long TTL_MASK = 0x8000000000000000L;
+
+ public static OldEphemeralType get(long ephemeralOwner) {
--- End diff --
+1, I like @anmolnar's idea of keeping `EphemeralTypeEmulate353` class name as this class is a 3.5.3 specific item. `Old` may create confusions later if we couldn't remove it in 3.5.5 or so, right? Better I would suggest to rename it as we are creating newly:-)
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r169985373
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -37,41 +77,152 @@
/**
* TTL node
*/
- TTL;
+ TTL() {
+ @Override
+ public long maxValue() {
+ return EXTENDED_FEATURE_VALUE_MASK; // 12725 days, about 34 years
+ }
+
+ @Override
+ public long toEphemeralOwner(long ttl) {
+ if ((ttl > TTL.maxValue()) || (ttl <= 0)) {
+ throw new IllegalArgumentException("ttl must be positive and cannot be larger than: " + TTL.maxValue());
+ }
+ //noinspection PointlessBitwiseExpression
+ return EXTENDED_MASK | EXTENDED_BIT_TTL | ttl; // TTL_RESERVED_BIT is actually zero - but it serves to document that the proper extended bit needs to be set
+ }
+
+ @Override
+ public long getValue(long ephemeralOwner) {
+ return getExtendedFeatureValue(ephemeralOwner);
+ }
+ };
+
+ /**
+ * For types that support it, the maximum extended value
+ *
+ * @return 0 or max
+ */
+ public long maxValue() {
+ return 0;
+ }
+
+ /**
+ * For types that support it, convert a value to an extended ephemeral owner
+ *
+ * @return 0 or extended ephemeral owner
+ */
+ public long toEphemeralOwner(long value) {
+ return 0;
+ }
+
+ /**
+ * For types that support it, return the extended value from an extended ephemeral owner
+ *
+ * @return 0 or extended value
+ */
+ public long getValue(long ephemeralOwner) {
+ return 0;
+ }
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_EXTENDED_SERVER_ID = 0xfe; // 254
+
+ private static final long EXTENDED_MASK = 0xff00000000000000L;
+ private static final long EXTENDED_BIT_TTL = 0x0000;
+ private static final long RESERVED_BITS_MASK = 0x00ffff0000000000L;
+ private static final long RESERVED_BITS_SHIFT = 40;
+
+ private static final Map<Long, EphemeralType> extendedFeatureMap;
+ static {
+ Map<Long, EphemeralType> map = new HashMap<>();
+ map.put(EXTENDED_BIT_TTL, TTL);
+ extendedFeatureMap = Collections.unmodifiableMap(map);
+ }
+
+ private static final long EXTENDED_FEATURE_VALUE_MASK = ~(EXTENDED_MASK | RESERVED_BITS_MASK);
+
+ // Visible for testing
+ static final String EXTENDED_TYPES_ENABLED_PROPERTY = "zookeeper.extendedTypesEnabled";
+ static final String TTL_3_5_3_EMULATION_PROPERTY = "zookeeper.emulate353TTLNodes";
+
+ /**
+ * Return true if extended ephemeral types are enabled
+ *
+ * @return true/false
+ */
+ public static boolean extendedEphemeralTypesEnabled() {
--- End diff --
The way it's implemented now, we can add easily new features in the future. Why code ourselves into a corner when we can leave some room? BTW - I'd discussed this offline with @phunt
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r141521002
--- Diff: src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java ---
@@ -84,6 +84,9 @@ public static long initializeNextSession(long id) {
long nextSid;
nextSid = (Time.currentElapsedTime() << 24) >>> 8;
nextSid = nextSid | (id <<56);
+ if (nextSid == EphemeralType.CONTAINER_EPHEMERAL_OWNER) {
+ ++nextSid; // this is an unlikely edge case, but check it just in case
--- End diff --
"no longer strictly true" - actually it still is true. If `Time.currentElapsedTime()` returns 0 and server Id is 0x80, this will happen. That's why nextSid has to be incremented in that case.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r186422920
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -950,6 +952,39 @@ server.3=zoo3:2888:3888</programlisting>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>zookeeper.extendedTypesEnabled</term>
+
+ <listitem>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.extendedTypesEnabled</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Define to "true" to enable
+ extended features such as the creation of <ulink url="zookeeperProgrammers.html#TTL+Nodes">TTL Nodes</ulink>.
+ They are disabled by default. IMPORTANT: when enabled server IDs must
+ be less than 255 due to internal limitations.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>zookeeper.emulate353TTLNodes</term>
+
+ <listitem>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.emulate353TTLNodes</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Due to
+ <ulink url="https://issues.apache.org/jira/browse/ZOOKEEPER-2901">ZOOKEEPER-2901</ulink> TTL nodes
+ created in version 3.5.3 are not supported in 3.5.4/3.6.0. However, a workaround is provided via the
+ zookeeper.emulate353TTLNodes system property. If you used TTL nodes in ZooKeeper 3.5.3 and need to maintain
+ compatibility set <emphasis role="bold">zookeeper.emulate353TTLNodes</emphasis> to "true" in addition to
+ <emphasis role="bold">zookeeper.extendedTypesEnabled</emphasis>. NOTE: due to the bug, server IDs
+ must be 127 or less. Additionally, the maximum support TTL value is 1099511627775 which is smaller
+ than what was allowed in 3.5.3 (1152921504606846975)</para>
--- End diff --
I think, the max limit "1152921504606846975" was not documented in 3.5.3 doc. Please add unit `milliseconds` to the value. Also, please take care @phunt's comment.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r153668593
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -40,19 +39,21 @@
TTL;
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_TTL = 0x00ffffffffffffffL;
+ public static final long TTL_MASK = 0xff00000000000000L;
+ public static final long MAX_TTL_SERVER_ID = 0xfe; // 254
public static EphemeralType get(long ephemeralOwner) {
if (ephemeralOwner == CONTAINER_EPHEMERAL_OWNER) {
return CONTAINER;
}
- if (ephemeralOwner < 0) {
+ if ((ephemeralOwner & TTL_MASK) == TTL_MASK) {
--- End diff --
I think this is broken. If, for example, org.apache.zookeeper.server.DataTree#deleteNode calls this method with TTL turned off, but using a server id of 255 it will end up calling this code
} else if (ephemeralType == EphemeralType.TTL) {
ttls.remove(path);
and the node will not be removed properly in deleteNode.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
My "review" was a question. :-) Honestly I'm not sure what is the right thing to do here. Afraid it's painted itself into a bit of corner.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r169984867
--- Diff: src/java/main/org/apache/zookeeper/server/OldEphemeralType.java ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+/**
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-2901
+ *
+ * version 3.5.3 introduced bugs associated with how TTL nodes were implemented. version 3.5.4
+ * fixes the problems but makes TTL nodes created in 3.5.3 invalid. OldEphemeralType is a copy
+ * of the old - bad - implementation that is provided as a workaround. {@link EphemeralType#TTL_3_5_3_EMULATION_PROPERTY}
+ * can be used to emulate support of the badly specified TTL nodes.
+ */
+public enum OldEphemeralType {
+ /**
+ * Not ephemeral
+ */
+ VOID,
+ /**
+ * Standard, pre-3.5.x EPHEMERAL
+ */
+ NORMAL,
+ /**
+ * Container node
+ */
+ CONTAINER,
+ /**
+ * TTL node
+ */
+ TTL;
+
+ public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
+ public static final long MAX_TTL = 0x0fffffffffffffffL;
+ public static final long TTL_MASK = 0x8000000000000000L;
+
+ public static OldEphemeralType get(long ephemeralOwner) {
--- End diff --
We should create a new task to delete it after a while. However, I know that this will be important. Whoever used TTLs in 3.5.3 will run into problems when they upgrade. We need to have a workaround for these users.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by DanBenediktson <gi...@git.apache.org>.
Github user DanBenediktson commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r146089767
--- Diff: conf/zoo_sample.cfg ---
@@ -6,6 +6,10 @@ initLimit=10
# The number of ticks that can pass between
# sending a request and getting an acknowledgement
syncLimit=5
+# enable TTL Nodes
+# IMPORTANT: when enabled, your server ID cannot be greater than 254
--- End diff --
Same comment I left on the JIRA ticket: shouldn't it be 127, not 254? My understanding of the problem was that the whole high bit was being used, not the whole byte sequence of 255.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r153668817
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -40,19 +39,21 @@
TTL;
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_TTL = 0x00ffffffffffffffL;
+ public static final long TTL_MASK = 0xff00000000000000L;
+ public static final long MAX_TTL_SERVER_ID = 0xfe; // 254
public static EphemeralType get(long ephemeralOwner) {
if (ephemeralOwner == CONTAINER_EPHEMERAL_OWNER) {
return CONTAINER;
}
- if (ephemeralOwner < 0) {
+ if ((ephemeralOwner & TTL_MASK) == TTL_MASK) {
--- End diff --
In particular you need to add a bunch more testing to ensure this case is working properly.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r148792118
--- Diff: conf/zoo_sample.cfg ---
@@ -6,6 +6,10 @@ initLimit=10
# The number of ticks that can pass between
# sending a request and getting an acknowledgement
syncLimit=5
+# enable TTL Nodes
+# IMPORTANT: when enabled, your server ID cannot be greater than 254
--- End diff --
@DanBenediktson 127 is the case with the existing bug. I re-wrote how this is handled so that now it's 255. The high bit is now used to signal that the ephemeralOwner is special.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r141521029
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -125,6 +125,8 @@
private final ZooKeeperServerListener listener;
private ZooKeeperServerShutdownHandler zkShutdownHandler;
+ private volatile boolean defaultTtlNodesEnabled = true;
--- End diff --
If it's false, then TTL nodes won't be available in Standalone mode.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r146086967
--- Diff: conf/zoo_sample.cfg ---
@@ -6,6 +6,10 @@ initLimit=10
# The number of ticks that can pass between
# sending a request and getting an acknowledgement
syncLimit=5
+# enable TTL Nodes
+# IMPORTANT: when enabled, your server ID cannot be greater than 254
+# See https://zookeeper.apache.org/doc/r3.5.3-beta/zookeeperAdmin.html#sc_configuration for details
--- End diff --
Perhaps note the document and section, rather than the link? It's going to get out of date quickly.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r172509908
--- Diff: src/java/main/org/apache/zookeeper/server/OldEphemeralType.java ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+/**
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-2901
+ *
+ * version 3.5.3 introduced bugs associated with how TTL nodes were implemented. version 3.5.4
+ * fixes the problems but makes TTL nodes created in 3.5.3 invalid. OldEphemeralType is a copy
+ * of the old - bad - implementation that is provided as a workaround. {@link EphemeralType#TTL_3_5_3_EMULATION_PROPERTY}
+ * can be used to emulate support of the badly specified TTL nodes.
+ */
+public enum OldEphemeralType {
+ /**
+ * Not ephemeral
+ */
+ VOID,
+ /**
+ * Standard, pre-3.5.x EPHEMERAL
+ */
+ NORMAL,
+ /**
+ * Container node
+ */
+ CONTAINER,
+ /**
+ * TTL node
+ */
+ TTL;
+
+ public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
+ public static final long MAX_TTL = 0x0fffffffffffffffL;
+ public static final long TTL_MASK = 0x8000000000000000L;
+
+ public static OldEphemeralType get(long ephemeralOwner) {
--- End diff --
Makes sense.
I think it would be slightly more accurate to name the old enum to `EphemeralTypeEmu353`.
What do you think?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163747537
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
That said - I could probably do it. Let me know. If you think it's important I'll see what I can do.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt I've been trying to focus on getting the branches, jenkins, etc... back in shape so that I can cut some releases. Any chance your original collaborators can help? I'm kinda swamped and just getting up to speed on these will take time...
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
The zoo_sample.cfg file should also be updated as part of this patch. It should include the option, and IMO it should have ttl turned on by default (similar warning wrt id as comment in the file?)
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r159091284
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
@phunt I'm not sure what you mean about quorum peer. I could find another way to set this value. Also, it's only for testing and clearly marked. What do you suggest?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163420180
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
I looked at it a bit originally, iirc I figured that you'd pass it through in some way (during instance creation) rather than making it a global.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r170009123
--- Diff: src/java/main/org/apache/zookeeper/server/OldEphemeralType.java ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+/**
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-2901
+ *
+ * version 3.5.3 introduced bugs associated with how TTL nodes were implemented. version 3.5.4
+ * fixes the problems but makes TTL nodes created in 3.5.3 invalid. OldEphemeralType is a copy
+ * of the old - bad - implementation that is provided as a workaround. {@link EphemeralType#TTL_3_5_3_EMULATION_PROPERTY}
+ * can be used to emulate support of the badly specified TTL nodes.
+ */
+public enum OldEphemeralType {
+ /**
+ * Not ephemeral
+ */
+ VOID,
+ /**
+ * Standard, pre-3.5.x EPHEMERAL
+ */
+ NORMAL,
+ /**
+ * Container node
+ */
+ CONTAINER,
+ /**
+ * TTL node
+ */
+ TTL;
+
+ public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
+ public static final long MAX_TTL = 0x0fffffffffffffffL;
+ public static final long TTL_MASK = 0x8000000000000000L;
+
+ public static OldEphemeralType get(long ephemeralOwner) {
--- End diff --
I put it in `OldEphemeralType` so it's easier to remove and reason about. The emulation is a separate concern.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@phunt you reviewed this PR originally so I was hoping you'd merge. @rgs1 can you merge if Pat can't?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r169865059
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -37,41 +77,152 @@
/**
* TTL node
*/
- TTL;
+ TTL() {
+ @Override
+ public long maxValue() {
+ return EXTENDED_FEATURE_VALUE_MASK; // 12725 days, about 34 years
+ }
+
+ @Override
+ public long toEphemeralOwner(long ttl) {
+ if ((ttl > TTL.maxValue()) || (ttl <= 0)) {
+ throw new IllegalArgumentException("ttl must be positive and cannot be larger than: " + TTL.maxValue());
+ }
+ //noinspection PointlessBitwiseExpression
+ return EXTENDED_MASK | EXTENDED_BIT_TTL | ttl; // TTL_RESERVED_BIT is actually zero - but it serves to document that the proper extended bit needs to be set
+ }
+
+ @Override
+ public long getValue(long ephemeralOwner) {
+ return getExtendedFeatureValue(ephemeralOwner);
+ }
+ };
+
+ /**
+ * For types that support it, the maximum extended value
+ *
+ * @return 0 or max
+ */
+ public long maxValue() {
+ return 0;
+ }
+
+ /**
+ * For types that support it, convert a value to an extended ephemeral owner
+ *
+ * @return 0 or extended ephemeral owner
+ */
+ public long toEphemeralOwner(long value) {
+ return 0;
+ }
+
+ /**
+ * For types that support it, return the extended value from an extended ephemeral owner
+ *
+ * @return 0 or extended value
+ */
+ public long getValue(long ephemeralOwner) {
+ return 0;
+ }
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_EXTENDED_SERVER_ID = 0xfe; // 254
+
+ private static final long EXTENDED_MASK = 0xff00000000000000L;
+ private static final long EXTENDED_BIT_TTL = 0x0000;
+ private static final long RESERVED_BITS_MASK = 0x00ffff0000000000L;
+ private static final long RESERVED_BITS_SHIFT = 40;
+
+ private static final Map<Long, EphemeralType> extendedFeatureMap;
+ static {
+ Map<Long, EphemeralType> map = new HashMap<>();
+ map.put(EXTENDED_BIT_TTL, TTL);
+ extendedFeatureMap = Collections.unmodifiableMap(map);
+ }
+
+ private static final long EXTENDED_FEATURE_VALUE_MASK = ~(EXTENDED_MASK | RESERVED_BITS_MASK);
+
+ // Visible for testing
+ static final String EXTENDED_TYPES_ENABLED_PROPERTY = "zookeeper.extendedTypesEnabled";
+ static final String TTL_3_5_3_EMULATION_PROPERTY = "zookeeper.emulate353TTLNodes";
+
+ /**
+ * Return true if extended ephemeral types are enabled
+ *
+ * @return true/false
+ */
+ public static boolean extendedEphemeralTypesEnabled() {
--- End diff --
Maybe I'm missing the point here. Would you please elaborate a little bit on what's the additional benefit of making this generic by extended ephemeral types?
Why don't you just KISS (keep it simple), because YAGNI (you ain't gonna need) the extended types?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r156808074
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -949,14 +949,15 @@ server.3=zoo3:2888:3888</programlisting>
</varlistentry>
<varlistentry>
- <term>ttlNodesEnabled</term>
+ <term>zookeeper.extendedTypesEnabled</term>
--- End diff --
Do we are have any room left for this? iiuc ttls are the last.
My concern here - won't people be confused by this, e.g. "are containers extended types"?
What do you think @Randgalt ?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r146087059
--- Diff: conf/zoo_sample.cfg ---
@@ -6,6 +6,10 @@ initLimit=10
# The number of ticks that can pass between
# sending a request and getting an acknowledgement
syncLimit=5
+# enable TTL Nodes
+# IMPORTANT: when enabled, your server ID cannot be greater than 254
+# See https://zookeeper.apache.org/doc/r3.5.3-beta/zookeeperAdmin.html#sc_configuration for details
+ttlNodesEnabled=false
--- End diff --
I was actually thinking that this specific line (not in general) should be true by default. That way anyone starting out would get the feature enabled by default - and they wouldn't have any legacy serverids specified. What do you think?
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@phunt Can we get this merged please?
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r166787684
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -950,6 +952,39 @@ server.3=zoo3:2888:3888</programlisting>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>zookeeper.extendedTypesEnabled</term>
+
+ <listitem>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.extendedTypesEnabled</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Define to "true" to enable
+ extended features such as the creation of <ulink url="zookeeperProgrammers.html#TTL+Nodes">TTL Nodes</ulink>.
+ They are disabled by default. IMPORTANT: when enabled server IDs must
+ be less than 255 due to internal limitations.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>zookeeper.emulate353TTLNodes</term>
+
+ <listitem>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.emulate353TTLNodes</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Due to
+ <ulink url="https://issues.apache.org/jira/browse/ZOOKEEPER-2901">ZOOKEEPER-2901</ulink> TTL nodes
+ created in version 3.5.3 are not supported in 3.5.4/3.6.0. However, a workaround is provided via the
+ zookeeper.emulate353TTLNodes system property. If you used TTL nodes in ZooKeeper 3.5.3 and need to maintain
+ compatibility set <emphasis role="bold">zookeeper.emulate353TTLNodes</emphasis> to "true" in addition to
+ <emphasis role="bold">zookeeper.extendedTypesEnabled</emphasis>. NOTE: due to the bug, server IDs
+ must be 127 or less. Additionally, the maximum support TTL value is 1099511627775 which is smaller
+ than what was allowed in 3.5.3 (1152921504606846975)</para>
--- End diff --
Is there documentation around this? I'd recommend documenting the bounds.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
This feature is ready to merge (along with #378)
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt i'd really like to push out a 3.5.4 - do you think it would make sense to release note this in 3.5.4 and address for 3.5.5? If you think we can finalize this PR soon I'm still open to that.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/377
@Randgalt Not strictly part of this PR, but I noticed that ContainerManager doesn't log the name of the container being deleted here:
```java
try {
LOG.info("Attempting to delete candidate container: %s",
containerPath);
requestProcessor.processRequest(request);
} catch (Exception e) {
LOG.error(String.format("Could not delete container: %s" ,
containerPath), e);
}
```
The `%s` should be `{}`.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163671234
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
As I recall that was extremely difficult. Config is not shared between different parts of the code. The only way to do that type of thing is via System defines. That's why I did this.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r141200124
--- Diff: src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java ---
@@ -84,6 +84,9 @@ public static long initializeNextSession(long id) {
long nextSid;
nextSid = (Time.currentElapsedTime() << 24) >>> 8;
nextSid = nextSid | (id <<56);
+ if (nextSid == EphemeralType.CONTAINER_EPHEMERAL_OWNER) {
+ ++nextSid; // this is an unlikely edge case, but check it just in case
--- End diff --
This makes me nervous but I can't think of a reason why it wont work. It may be worth changing the comment on this method because it is no longer strictly true.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r186415246
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -950,6 +952,39 @@ server.3=zoo3:2888:3888</programlisting>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>zookeeper.extendedTypesEnabled</term>
+
+ <listitem>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.extendedTypesEnabled</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Define to "true" to enable
+ extended features such as the creation of <ulink url="zookeeperProgrammers.html#TTL+Nodes">TTL Nodes</ulink>.
+ They are disabled by default. IMPORTANT: when enabled server IDs must
+ be less than 255 due to internal limitations.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>zookeeper.emulate353TTLNodes</term>
--- End diff --
Yes, good point. We need to mention this as `deprecated `and highlight the intention of removing them in future. As per "https://cwiki.apache.org/confluence/display/ZOOKEEPER/Roadmap", are we able to remove it in 3.5.5 ?
Also, in future how user with TTLNodes can be upgraded to a to a higher 3.5.5+ or 3.6.0+ version directly from 3.5.3 ?
Also, could you please raise a jira task to propose the best way to remove this workaround config and keep track of it, if not raised yet.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163419925
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -40,19 +39,40 @@
TTL;
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_TTL = 0x00ffffffffffffffL;
+ public static final long TTL_MASK = 0xff00000000000000L;
+ public static final long MAX_TTL_SERVER_ID = 0xfe; // 254
+
+ public static final String EXTENDED_TYPES_ENABLED_PROPERTY = "zookeeper.extendedTypesEnabled";
+
+ public static boolean extendedEphemeralTypesEnabled() {
+ return Boolean.getBoolean(EXTENDED_TYPES_ENABLED_PROPERTY);
+ }
public static EphemeralType get(long ephemeralOwner) {
+ if ( extendedEphemeralTypesEnabled() ) {
+ if ((ephemeralOwner & TTL_MASK) == TTL_MASK) {
--- End diff --
We discussed this earlier in the PR. Problem is it's b/w incompat one way or the other. Given serverid has been around forever, and TTL only added in 3.5.3 (a beta) we went forward with this approach.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r156821102
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
This is really ugly. Why not using a quorum peer instead? Given you want to control the serverid.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
> I used the sample zoo.cfg file as my config and started via bin/zkServer.sh - however I can still create a ttl based node even though "ttlNodesEnabled=false" in the zoo.cfg. I recommend you add a test for this.
@phunt I imagine the server started in Standalone mode. Please verify. When the server starts in Standalone mode it ignores most of zoo.cfg. Also in Standalone mode there is no Server ID so it's a non issue. If you add `server.X` and `standaloneEnabled=false` to zoo.cfg then the ttlNodesEnabled has effect. Make sense? Does this need to be documented?
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/377
I think it needs to do the same thing whether it's in standalone mode or not. Least surprise. Also folks might want to test in standalone mode with the same basic configuration that they use with an ensemble size > 1. They'd want the same behavior. I'd say it should work the same regardless whether it's standalone or not.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r166787489
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -950,6 +952,39 @@ server.3=zoo3:2888:3888</programlisting>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>zookeeper.extendedTypesEnabled</term>
+
+ <listitem>
+ <para>(Java system property only: <emphasis
+ role="bold">zookeeper.extendedTypesEnabled</emphasis>)</para>
+
+ <para><emphasis role="bold">New in 3.5.4, 3.6.0:</emphasis> Define to "true" to enable
+ extended features such as the creation of <ulink url="zookeeperProgrammers.html#TTL+Nodes">TTL Nodes</ulink>.
+ They are disabled by default. IMPORTANT: when enabled server IDs must
+ be less than 255 due to internal limitations.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>zookeeper.emulate353TTLNodes</term>
--- End diff --
What's the plan for deprecating this? Keeping this parameter around forever seems like a bad idea. Adds complication that we don't really need to carry around. Perhaps we can deprecate in 3.5.4 and remove in 3.5.5? Similarly I don't think we should include this at all in 3.6.0 (trunk).
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/377
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
The integration above doesn't seem to update, but the build passes now.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/377
Never mind. I'll create a separate PR for that.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163712641
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
It being a global seems to kick the problem down the road to the next person. I'm worried that setting this jvm wide could have an impact when developing tests in the future. That's why it caught my eye.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r159091495
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml ---
@@ -949,14 +949,15 @@ server.3=zoo3:2888:3888</programlisting>
</varlistentry>
<varlistentry>
- <term>ttlNodesEnabled</term>
+ <term>zookeeper.extendedTypesEnabled</term>
--- End diff --
I only thought that we might be able to use the setting for other things in the future. But, I'm OK either way. Let me know.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
@phunt it's back to default false now. So, I hope this can be merged.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
Controlling TTL Nodes via zoo.cfg turned out to be untennable. There are too many parts of the code that need to know about TTLs being enabled or not. The previous PR had several holes relating to this. This new commit changes the enabled/disabled mechanism to be a System property so that it can be accessed anywhere in the code. It's also been renamed something more general so that it can be applied to future features and not just TTLs.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r161146799
--- Diff: src/java/main/org/apache/zookeeper/server/EphemeralType.java ---
@@ -40,19 +39,40 @@
TTL;
public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
- public static final long MAX_TTL = 0x0fffffffffffffffL;
- public static final long TTL_MASK = 0x8000000000000000L;
+ public static final long MAX_TTL = 0x00ffffffffffffffL;
+ public static final long TTL_MASK = 0xff00000000000000L;
+ public static final long MAX_TTL_SERVER_ID = 0xfe; // 254
+
+ public static final String EXTENDED_TYPES_ENABLED_PROPERTY = "zookeeper.extendedTypesEnabled";
+
+ public static boolean extendedEphemeralTypesEnabled() {
+ return Boolean.getBoolean(EXTENDED_TYPES_ENABLED_PROPERTY);
+ }
public static EphemeralType get(long ephemeralOwner) {
+ if ( extendedEphemeralTypesEnabled() ) {
+ if ((ephemeralOwner & TTL_MASK) == TTL_MASK) {
--- End diff --
To keep backward compatible, shouldn't we keep use 0x80 as the TTL byte?
People may not use sid larger than 127, it's more likely they will use the TTL with 0x80 highest byte. With this change, those nodes won't be expired anymore.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r170008461
--- Diff: src/java/main/org/apache/zookeeper/server/OldEphemeralType.java ---
@@ -0,0 +1,74 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+/**
+ * See https://issues.apache.org/jira/browse/ZOOKEEPER-2901
+ *
+ * version 3.5.3 introduced bugs associated with how TTL nodes were implemented. version 3.5.4
+ * fixes the problems but makes TTL nodes created in 3.5.3 invalid. OldEphemeralType is a copy
+ * of the old - bad - implementation that is provided as a workaround. {@link EphemeralType#TTL_3_5_3_EMULATION_PROPERTY}
+ * can be used to emulate support of the badly specified TTL nodes.
+ */
+public enum OldEphemeralType {
+ /**
+ * Not ephemeral
+ */
+ VOID,
+ /**
+ * Standard, pre-3.5.x EPHEMERAL
+ */
+ NORMAL,
+ /**
+ * Container node
+ */
+ CONTAINER,
+ /**
+ * TTL node
+ */
+ TTL;
+
+ public static final long CONTAINER_EPHEMERAL_OWNER = Long.MIN_VALUE;
+ public static final long MAX_TTL = 0x0fffffffffffffffL;
+ public static final long TTL_MASK = 0x8000000000000000L;
+
+ public static OldEphemeralType get(long ephemeralOwner) {
--- End diff --
Yes, that's fine with the emulate353 flag.
My concern is that we keep 2 enums in the codebase: `EphemeralType` and `OldEphemeralType` while I think it'd be nicer to build the functionality of the old enum into the new one.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r163746348
--- Diff: src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java ---
@@ -476,9 +474,12 @@ public ZooKeeperServerListener getZooKeeperServerListener() {
return listener;
}
+ // Visible for testing
+ static volatile int serverId = 1;
--- End diff --
Here's the issue @phunt - `serverId` is consumed by `ZooKeeperServer.createSessionTracker()`. For tests, the ZooKeeperServer is created by the **static** method `ClientBase.startServerInstance()`. So, _I'm_ the person down the road that has to deal with the static method created long ago. It would take significant re-write for `ClientBase.startServerInstance()` to be non-static and parameterized.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/zookeeper/pull/377
FYI - I just pushed a change that adds yet-another-flag that allows 3.5.4 ZKs to read the old 3.5.3 TTL nodes. I think we must have this. The docs are updated too.
---
[GitHub] zookeeper pull request #377: [ZOOKEEPER-2901] TTL Nodes don't work with Serv...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/377#discussion_r156821209
--- Diff: src/docs/src/documentation/content/xdocs/zookeeperProgrammers.xml ---
@@ -271,9 +271,9 @@
is not modified within the TTL and has no children it will become a candidate
to be deleted by the server at some point in the future.</para>
- <para>Note: TTL Nodes must be enabled in your ZooKeeper configuration file as
+ <para>Note: TTL Nodes must be enabled via System property as
--- End diff --
and this.
---
[GitHub] zookeeper issue #377: [ZOOKEEPER-2901] TTL Nodes don't work with Server IDs ...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/377
@afine This one is also an important PR to review and merge if you have a chance. I think it's already in a good shape.
---