-
Notifications
You must be signed in to change notification settings - Fork 7.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeeper service unavailable #1080
Changes from 1 commit
6fe2712
4174ba7
a22341d
2a009dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,14 @@ public String readString(String tag) throws IOException { | |
} | ||
|
||
static public final int maxBuffer = Integer.getInteger("jute.maxbuffer", 0xfffff); | ||
static public int readExtraSize = Integer.getInteger("zookeeper.jute.maxbuffer.extrasize", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please make it final There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. variable readExtraSize is reinitialized at line number 93. So I think it cannot be marked final. |
||
maxBuffer); | ||
static { | ||
// Earlier hard coded value is 1024, So the value should not be less than that | ||
if (readExtraSize < 1024) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assign an initial value to a temp variable here, do this small computation, then assign the value to the variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, changed it |
||
readExtraSize = 1024; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a reasonable estimate of an upper bound of the extra field padding which could reasonably cover more practice use cases, so users don't have to configure this value themselves? Obviously it's not tractable to find a single value that cover all cases (unless we use infinity), but it seems reasonable to increase this value - say by 10x while still be safeguarding requests with unreasonable length. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this patch we have changed the default value of extra size to same as max buffer value. I do not foresee any scenario where record extra size is more than the actual record size. |
||
} | ||
} | ||
|
||
public byte[] readBuffer(String tag) throws IOException { | ||
int len = readInt(tag); | ||
|
@@ -123,7 +131,7 @@ public void endMap(String tag) throws IOException {} | |
// make up for extra fields, etc. (otherwise e.g. clients may be able to | ||
// write buffers larger than we can read from disk!) | ||
private void checkLength(int len) throws IOException { | ||
if (len < 0 || len > maxBuffer + 1024) { | ||
if (len < 0 || len > maxBuffer + readExtraSize) { | ||
throw new IOException(UNREASONBLE_LENGTH + len); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,146 @@ | ||
/** | ||
* 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 | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* 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; | ||
|
||
import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT; | ||
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.junit.Assert.fail; | ||
|
||
import java.io.File; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
import org.apache.zookeeper.ZooDefs.Ids; | ||
import org.apache.zookeeper.common.ZKConfig; | ||
import org.apache.zookeeper.server.ZKDatabase; | ||
import org.apache.zookeeper.server.persistence.FileTxnSnapLog; | ||
import org.apache.zookeeper.server.quorum.QuorumPeerTestBase; | ||
import org.apache.zookeeper.test.ClientBase; | ||
import org.junit.After; | ||
import org.junit.Assert; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
|
||
public class JuteMaxBufferTest extends QuorumPeerTestBase { | ||
private MainThread mt; | ||
|
||
@Before | ||
public void setup() throws Exception { | ||
// Request size for 100 nodes in this test class is 6197 bytes | ||
System.setProperty(ZKConfig.JUTE_MAXBUFFER, Integer.toString(6197)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This trick won't work, because the variable will be inizialized only at the first loading of the class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ZKConfig.JUTE_MAXBUFFER is initialized with only one value that is why it was working, anyway I moved initialization to class level so it is initialized only once There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @arshadmohammad if you run this test together with the other ones you will see that the system property does not have effect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @eolivelli for your suggestions. I had run this test in ant build setup and it was working fine. I think in ant it was forking new jvm for every test class. that is why it might have passed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what shall we do, shall we commit without test case? any other idea? |
||
} | ||
|
||
/** | ||
* ZOOKEEPER-3496 | ||
*/ | ||
@Test | ||
public void testServerAllowsTransactionMoreThanMaxBufferSize() throws Exception { | ||
// in this test case, multi operation request size is 6196 bytes | ||
int clientPort = PortAssignment.unique(); | ||
String quorumCfgSection = "server.1=127.0.0.1:" + (PortAssignment.unique()) + ":" | ||
+ (PortAssignment.unique()) + ":participant;" + clientPort + "\n"; | ||
|
||
mt = new MainThread(1, clientPort, quorumCfgSection, false); | ||
mt.start(); | ||
Assert.assertTrue("waiting for server 1 being up", | ||
ClientBase.waitForServerUp("127.0.0.1:" + clientPort, CONNECTION_TIMEOUT)); | ||
|
||
ZooKeeper zk = ClientBase.createZKClient("127.0.0.1:" + clientPort); | ||
String parent = "/parent"; | ||
zk.create(parent, "".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); | ||
zk.addAuthInfo("digest", "pat:test".getBytes()); | ||
List<Op> ops = new ArrayList<Op>(); | ||
int numberOfNodes = 100; | ||
for (int i = 0; i < numberOfNodes; i++) { | ||
ops.add(Op.create(parent + "/child" + i, ("data" + i).getBytes(), Ids.CREATOR_ALL_ACL, | ||
CreateMode.PERSISTENT)); | ||
} | ||
/** | ||
* Have set jute.maxbuffer to 6197. 100 znode size is 6197, so creation will be | ||
* success | ||
*/ | ||
zk.multi(ops); | ||
List<String> children = zk.getChildren(parent, false); | ||
// check nodes are created successfully. | ||
assertEquals(numberOfNodes, children.size()); | ||
|
||
/** | ||
* Server added some additional information(ACL in this case) in those 100 | ||
* znodes. So total size of 100 records in transaction log file is more than | ||
* 6197. Total size is around 9616. So it added 3419 extra bytes. If extrasize | ||
* is kept 1024, then test case will fail (earlier default scenario). Now the | ||
* extraSize default value is same as jute buffer size. In this case it will be | ||
* 6197. So 9616 less than (6197+6197) will pass. | ||
*/ | ||
File dataDir = new File(mt.getConfFile().getParentFile(), "data"); | ||
assertTrue("data directory does not exist", dataDir.exists()); | ||
ZKDatabase database = new ZKDatabase(new FileTxnSnapLog(dataDir, dataDir)); | ||
database.loadDataBase(); | ||
} | ||
|
||
/** | ||
* ZOOKEEPER-3496. This test is normal jute.maxbuffer functionality test. It | ||
* should pass before and after fix | ||
*/ | ||
@Test | ||
public void testZKOperationRequestOfSizeGreaterThanMaxBuffer() throws Exception { | ||
int clientPort = PortAssignment.unique(); | ||
String quorumCfgSection = "server.1=127.0.0.1:" + (PortAssignment.unique()) + ":" | ||
+ (PortAssignment.unique()) + ":participant;" + clientPort + "\n"; | ||
|
||
mt = new MainThread(1, clientPort, quorumCfgSection, false); | ||
mt.start(); | ||
Assert.assertTrue("waiting for server 1 being up", | ||
ClientBase.waitForServerUp("127.0.0.1:" + clientPort, CONNECTION_TIMEOUT)); | ||
|
||
ZooKeeper zk = ClientBase.createZKClient("127.0.0.1:" + clientPort); | ||
String parent = "/parent"; | ||
zk.create(parent, "".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); | ||
zk.addAuthInfo("digest", "pat:test".getBytes()); | ||
List<Op> ops = new ArrayList<Op>(); | ||
|
||
/** | ||
* This code will create 101 Nodes , 100 node size is 6197 Bytes. So this | ||
* operation should fail | ||
*/ | ||
int numberOfNodes = 101; | ||
for (int i = 0; i < numberOfNodes; i++) { | ||
ops.add(Op.create(parent + "/child" + i, ("data" + i).getBytes(), Ids.CREATOR_ALL_ACL, | ||
CreateMode.PERSISTENT)); | ||
} | ||
try { | ||
zk.multi(ops); | ||
fail("KeeperException is expected as request size is more than jute.maxbuffer size"); | ||
} catch (KeeperException e) { | ||
System.out.println("Expected to fail as request size exceeded jute max buffer size" | ||
+ e.getMessage()); | ||
// do nothing, Exception is expected | ||
} | ||
} | ||
|
||
@After | ||
public void tearDown() throws Exception { | ||
System.clearProperty(ZKConfig.JUTE_MAXBUFFER); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved to class level clean up |
||
if (mt != null) { | ||
// do cleanup | ||
mt.shutdown(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to have better testing we can do as follows:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the approach. thanks. Addressed the comments more or less same way. Please have a look.
maxBuffer is used many places and it is not the default value, it is the configured value. So leaving this variable as it is. created other instance variable as suggested but with different name, hope it is ok.