You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "zichen-gan (via GitHub)" <gi...@apache.org> on 2024/05/16 03:16:36 UTC
[PR] Zookeeper 2789 [zookeeper]
zichen-gan opened a new pull request, #2164:
URL: https://github.com/apache/zookeeper/pull/2164
The original PR link: https://github.com/apache/zookeeper/pull/262
Since the aforementioned PR does not support rolling hot updates, this PR aims to add rolling upgrade capabilities. The goal of this PR is to change the counter bit length from 32 to 40 and the epoch bit length from 32 to 20 through a rolling upgrade approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2369626197
> Given above, I think it is promising. It promotes rollover rate from `49.7` days to `34.9` years assuming `1k/s` ops. The best is that it demands no protocol change at the price of zxid format change.
> Before finalizing this path, I may want to taste whether leadership inheritance is feasible.
Sure, I'll wait for your review.
Strange thing is that as you outlined it doesn't require protocol change, but still the patch has to increase protocol version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-4389983415
> Hi, any update on this PR? I'd love to see this fix land. If there's anything I can help with (rebasing, resolving conflicts, etc.), happy to pitch in.
Feel free to create a new pull request with changes rebased.
I'm not sure if original author @zichen-gan is still planning to show up.
@kezhuw thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2368107479
This pr will change the server side data format. I think it does not fit a patch release.
> If it is `1k/s` ops, then as long as $2^{32} / (86400 * 1000) \approx 49.7$ days ZXID will exhausted. https://github.com/apache/zookeeper/pull/262#issue-230567070
> Thinking about some abnormal situations, maybe 24 bit for epoch and 40 bit for counter is more better choice: M a t h . m i n ( 2 24 / ( 24 ∗ 365 ) , 2 40 / ( 86400 ∗ 1000 ∗ 365 ) ) ≈ M a t h . m i n ( 1915.2 , 34.9 ) = 34.9 years. https://github.com/apache/zookeeper/pull/262#issuecomment-303276951
> So i offered a better solution is 24-bit epoch in second comment. Even if the frequency of leader election is once by every single hours, we will not experience the epoch overflow until 1915.2 years later. https://github.com/apache/zookeeper/pull/262#issuecomment-351886573
Given above, I think it is promising. It promotes rollover rate from `49.7` days to `34.9` years assuming `1k/s` ops. The best is that it demands no protocol change at the price of zxid format change.
Before finalizing this path, I may want to taste whether leadership inheritance is feasible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "rooookkie (via GitHub)" <gi...@apache.org>.
rooookkie commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-4386532837
Hi, any update on this PR? I'd love to see this fix land. If there's anything I can help with (rebasing, resolving conflicts, etc.), happy to pitch in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#discussion_r1772292639
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java:
##########
@@ -525,15 +525,16 @@ public void run() {
long newEpoch = learnerMaster.getEpochToPropose(this.getSid(), lastAcceptedEpoch);
long newLeaderZxid = ZxidUtils.makeZxid(newEpoch, 0);
- if (this.getVersion() < 0x10000) {
- // we are going to have to extrapolate the epoch information
- long epoch = ZxidUtils.getEpochFromZxid(zxid);
- ss = new StateSummary(epoch, zxid);
- // fake the message
- learnerMaster.waitForEpochAck(this.getSid(), ss);
+ /**
+ * When the Leader switches to a new mode
+ * lower version Observers or Followers are not allowed to join the cluster, thus the zxid bit length has changed.
+ */
+ if (this.getVersion() < 0x11000) {
+ LOG.error("sid:{} version too old", this.sid);
+ return;
} else {
byte[] ver = new byte[4];
- ByteBuffer.wrap(ver).putInt(0x10000);
+ ByteBuffer.wrap(ver).putInt(0x11000);
Review Comment:
The patch doesn't alter anything in the protocol, but you still have to increase protocol version number in order to reject old followers. Odd, but seams reasonable.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java:
##########
@@ -1209,6 +1209,25 @@ private void loadDataBase() {
// load the epochs
long lastProcessedZxid = zkDb.getDataTree().lastProcessedZxid;
+ /**
+ * It is only after joining the cluster that we know whether the current use is 32-bit or 40-bit.
+ * Here, a new file is created locally to store the current number of epoch bits. This file will be created locally after upgrading the version to ensure a smooth joining of the cluster upon startup.
+ */
+ try {
+ currentEpochPosition = readLongFromFile(CURRENT_EPOCH_POSITION_FILENAME);
Review Comment:
Why do you need to store this information in a file?
Leader will set it automatically, learners will use the new version based on leader's version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2821532072
> on version 3.4.14, in which directory of Zookeeper is the complete installation package located
It is `zookeeper-assembly/target/apache-zookeeper-3.10.0-SNAPSHOT-bin.tar.gz` in master.
> I would like to verify this feature in our production environment.
Please backup your data. This implementation is a one way ticket, a.k.a. it changes data storage format and probably has no way to downgrade. I presented an alternative #2208(ZOOKEEPER-4883) before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-3609459806
@kezhuw Shall we merge this? Would you please resolve the conflict?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-3609512765
Does this patch add any reassignment capabilities to the `Zxid`?
Is it just about assigning the new bit length to existing value in a rolling upgrade fashion?
Why did you choose 40/20 instead of 48/16?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Zookeeper 2789 [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2362133733
@kezhuw @eolivelli @li4wang @ztzg
I've just come across this one and started to review for 3.9.3, but need more eyeballs. Could you please review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "wg1026688210 (via GitHub)" <gi...@apache.org>.
wg1026688210 commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2819936641
Hi~ @anmolnar @kezhuw I would like to verify this feature in our production environment. After running `mvn clean package -DskipTests` on version 3.4.14, in which directory of Zookeeper is the complete installation package located?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2370024980
> as you outlined it doesn't require protocol change, but still the patch has to increase protocol version.
My fault! By "no protocol change", I mean we don't need to prove its correctness in ZAB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-2789 Reassign `ZXID` for solving 32bit overflow problem [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2164:
URL: https://github.com/apache/zookeeper/pull/2164#issuecomment-2369624141
@zichen-gan You need to close / re-open PR or force push to trigger another CI run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org