ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeeper service unavailable #1080

arshadmohammad · 2019-09-05T07:52:07Z

No description provided.

…r unavailable

eolivelli · 2019-09-05T10:04:47Z

zookeeper-jute/src/main/java/org/apache/jute/BinaryInputArchive.java

@@ -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",


please make it final

variable readExtraSize is reinitialized at line number 93. So I think it cannot be marked final.

eolivelli · 2019-09-05T16:13:36Z

zookeeper-jute/src/main/java/org/apache/jute/BinaryInputArchive.java

+            maxBuffer);
+    static {
+        // Earlier hard coded value is 1024, So the value should not be less than that
+        if (readExtraSize < 1024) {


Assign an initial value to a temp variable here, do this small computation, then assign the value to the variable.
It is legal to assign a value in a static block to a final static field, the only constraint is that you do the assignement only once

ok, changed it

eolivelli · 2019-09-05T16:14:43Z

zookeeper-server/src/test/java/org/apache/zookeeper/JuteMaxBufferTest.java

+    @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));


This trick won't work, because the variable will be inizialized only at the first loading of the class.
Once you assign a value you cannot change it

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

@arshadmohammad if you run this test together with the other ones you will see that the system property does not have effect.
You can add an assertion in your test case that the value of readExtraSize is the expected value and it is different from the default one.

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.
But yes , if in one jvm multiple tests are running, value assigned to org.apache.jute.BinaryInputArchive#maxBuffer can not be changed from this test case.

So what shall we do, shall we commit without test case? any other idea?

eolivelli · 2019-09-05T16:14:58Z

zookeeper-server/src/test/java/org/apache/zookeeper/JuteMaxBufferTest.java

+
+    @After
+    public void tearDown() throws Exception {
+        System.clearProperty(ZKConfig.JUTE_MAXBUFFER);


moved to class level clean up

eolivelli

The change is okay.
We have to fix the test IMHO

eolivelli · 2019-09-06T15:38:36Z

zookeeper-server/src/test/java/org/apache/zookeeper/JuteMaxBufferTest.java

+    @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));


@arshadmohammad if you run this test together with the other ones you will see that the system property does not have effect.
You can add an assertion in your test case that the value of readExtraSize is the expected value and it is different from the default one.

eolivelli · 2019-09-06T15:40:07Z

The failure of Maven build is due to a problem on the C client, not related to this patch

client/zookeeper-client-c/generated -Wall -Werror -Wdeclaration-after-statement -fprofile-arcs -ftest-coverage -g -O2 -D_GNU_SOURCE -MT zookeeper.lo -MD -MP -MF .deps/zookeeper.Tpo -c /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/src/zookeeper.c  -fPIC -DPIC -o .libs/zookeeper.o
     [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/src/zookeeper.c: In function 'print_completion_queue':
     [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/src/zookeeper.c:2560:5: error: null argument where non-null required (argument 1) [-Werror=nonnull]
     [exec]      fprintf(LOGSTREAM,"Completion queue: ");
     [exec]      ^~~~~~~
     [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/src/zookeeper.c:2562:9: error: null argument where non-null required (argument 1) [-Werror=nonnull]
     [exec]          fprintf(LOGSTREAM,"empty\n");
     [exec]          ^~~~~~~
     [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/src/zookeeper.c:2568:9: error: null argument where non-null required (argument 1) [-Werror=nonnull]
     [exec]          fprintf(LOGSTREAM,"%d,",cptr->xid);
     [exec]          ^~~~~~~
     [exec] /home/jenkins/jenkins-slave/workspace/PreCommit-ZOOKEEPER-github-pr-build-maven/zookeeper-client/zookeeper-client-c/src/zookeeper.c:2571:5: error: null argument where non-null required (argument 1) [-Werror=nonnull]
     [exec]      fprintf(LOGSTREAM,"end\n");
     [exec]      ^~~~~~~
     [exec] Makefile:1032: recipe for target 'zookeeper.lo' failed
     [exec] cc1: all warnings being treated as errors
     [exec] make: *** [zookeeper.lo] Error 1
[INFO] ----------------------------------------------------

eolivelli · 2019-09-06T15:43:35Z

@arshadmohammad Do you want this fix to go in 3.5.6 ? I am going to cut the release, I can wait for you

eolivelli · 2019-09-06T15:48:40Z

Oh I see now that you are targeting branch 3.5.
Please target the PR to "master" then we will cherry-pick/backport to 3.5

arshadmohammad · 2019-09-07T05:53:50Z

@arshadmohammad Do you want this fix to go in 3.5.6 ? I am going to cut the release, I can wait for you

Please go ahead, we can take this issue in the next version

eolivelli · 2019-09-08T09:30:39Z

zookeeper-jute/src/main/java/org/apache/jute/BinaryInputArchive.java

@@ -85,6 +85,18 @@ public String readString(String tag) throws IOException {
    }

    static public final int maxBuffer = Integer.getInteger("jute.maxbuffer", 0xfffff);


In order to have better testing we can do as follows:

make maxBuffer and readExtraSize two instance variables

add two new constants (final static) DEFAULT_MAX_BUFFER and DEFAULT_READEXTRASIZE initialized with the system properties

in the constructor initialize the value using DEFAULT_MAX_BUFFER and DEFAULT_READEXTRASIZE

add a new constructor that allows to initialize maxBuffer and readExtraSize

add only unit tests about BinaryInputArchive, and not end to end tests

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.

hanm · 2019-09-12T06:06:43Z

zookeeper-jute/src/main/java/org/apache/jute/BinaryInputArchive.java

+            Integer.getInteger("zookeeper.jute.maxbuffer.extrasize", maxBuffer);
+        if (configuredValue < 1024) {
+            // Earlier hard coded value is 1024, So the value should not be less than that
+            readExtraSize = 1024;


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.

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.
So this value is going to be sufficient and possibly there would not be any need to configure jute.maxbuffer.extrasize.

hanm

I am wondering if we can precisely guarantee we are consistent on both read and write path for the requests. Currently, the extra padding approach is more empirical and it's still possible for any preconfigured value, the check will produce false negative results.

One way of doing this is to check serialized requests on write path so we know precisely the size of the request on write path, but that will be slower comparing to existing implementation.

arshadmohammad · 2019-09-20T07:19:28Z

I am wondering ....

Request actual size is known very late in the request processing chain. If very validate request size this time, purpose of sanity check is defeated..
If we try to create the request at the beginning of the request processing, i think sufficient data would not be available, also as you pointed out it would slower the performance for not good enough reasons.

eolivelli

LGTM

hanm · 2019-09-20T20:44:07Z

zookeeper-jute/src/main/java/org/apache/jute/BinaryInputArchive.java

+
+    static {
+        Integer configuredExtraMaxBuffer =
+            Integer.getInteger("zookeeper.jute.maxbuffer.extrasize", maxBuffer);


one suggestion: existing maxbuffer has a name of "jute.maxbuffer" (note, without zookeeper prefix), so it might be better
that we name this extra size property as "jute.maxbuffer.extrasize" to be consistent with existing style.

also, we need documentation on this new property.

There is big problem with jute.maxbuffer. It can not be configured in zoo.cfg. We are forced to configure it though java system property like -Djute.maxbuffer=someValue.

When we are giving property name zookeeper.jute.maxbuffer.extrasize. it can be configured in zoo.cfg as jute.maxbuffer.extrasize=someValue

This is the reason I do not want to be consistent with jute.maxbuffer. is it OK?

i see - sounds reasonable. I am ok with current name.

side note - I never really remember why we have a few configuration options that's only available through java system properties and what's the criteria of choosing one versus another.

hanm

lgtm (just need doc update).

hanm · 2019-09-23T20:18:12Z

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

+    While processing client requests ZooKeeper server adds some additional information into 
+    the requests before persisting it as a transaction. Earlier this additional information size 
+    was fixed to 1024 bytes. For many scenarios, specially scenarios where jute.maxbuffer value
+    is more than 1 MB and request type is multi, this fixed size was sufficient.


replace sufficient with insufficient ?

right, corrected it

lvfangmin

+1

Still need to tune the extra size when things changed, like a user have a lot more sub txns in a multi-op, but this seems better to tune the jute.maxbuffer which will also allow user to send more data.

…r service unavailable Author: Mohammad Arshad <arshad@apache.org> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Michael Han <hanm@apache.org>, Allan Lyu <fangmin@apache.org> Closes #1080 from arshadmohammad/ZOOKEEPER-3496

hanm · 2019-09-24T19:35:13Z

@arshadmohammad

can you also open a pull request to master branch? this pull request was targeting 3.5 instead of master and has a couple of merge conflicts with master branch.

this pul request is now merged to 3.5: 7d263d0, please close the pull request, thank you.

arshadmohammad · 2019-09-26T07:36:19Z

Thanks @eolivelli @hanm and @lvfangmin for the reviews.

ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeepe…

6fe2712

…r unavailable

eolivelli requested changes Sep 5, 2019

View reviewed changes

eolivelli requested changes Sep 6, 2019

View reviewed changes

eolivelli requested changes Sep 8, 2019

View reviewed changes

hanm reviewed Sep 12, 2019

View reviewed changes

Fixed review comments

4174ba7

eolivelli approved these changes Sep 20, 2019

View reviewed changes

hanm reviewed Sep 20, 2019

View reviewed changes

hanm approved these changes Sep 20, 2019

View reviewed changes

ZOOKEEPER-3496: Documented new configuration property

a22341d

hanm reviewed Sep 23, 2019

View reviewed changes

lvfangmin approved these changes Sep 23, 2019

View reviewed changes

ZOOKEEPER-3496: Fixed review comments

2a009dd

arshadmohammad closed this Sep 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeeper service unavailable #1080

ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeeper service unavailable #1080

arshadmohammad commented Sep 5, 2019

eolivelli Sep 5, 2019

arshadmohammad Sep 5, 2019 •

edited

eolivelli Sep 5, 2019

arshadmohammad Sep 6, 2019

eolivelli Sep 5, 2019

arshadmohammad Sep 6, 2019

eolivelli Sep 6, 2019

arshadmohammad Sep 6, 2019

arshadmohammad Sep 6, 2019

eolivelli Sep 5, 2019

arshadmohammad Sep 6, 2019

eolivelli left a comment

eolivelli Sep 6, 2019

eolivelli commented Sep 6, 2019

eolivelli commented Sep 6, 2019

eolivelli commented Sep 6, 2019

arshadmohammad commented Sep 7, 2019

eolivelli Sep 8, 2019

arshadmohammad Sep 20, 2019

hanm Sep 12, 2019

arshadmohammad Sep 20, 2019

hanm left a comment •

edited

arshadmohammad commented Sep 20, 2019

eolivelli left a comment

hanm Sep 20, 2019

arshadmohammad Sep 20, 2019

hanm Sep 20, 2019

hanm left a comment

hanm Sep 23, 2019

arshadmohammad Sep 24, 2019

lvfangmin left a comment

hanm commented Sep 24, 2019

arshadmohammad commented Sep 26, 2019

		@@ -85,6 +85,18 @@ public String readString(String tag) throws IOException {
		}

		static public final int maxBuffer = Integer.getInteger("jute.maxbuffer", 0xfffff);

ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeeper service unavailable #1080

ZOOKEEPER-3496: Transaction larger than jute.maxbuffer makes ZooKeeper service unavailable #1080

Conversation

arshadmohammad commented Sep 5, 2019

Choose a reason for hiding this comment

arshadmohammad Sep 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli commented Sep 6, 2019

eolivelli commented Sep 6, 2019

eolivelli commented Sep 6, 2019

arshadmohammad commented Sep 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanm left a comment • edited

Choose a reason for hiding this comment

arshadmohammad commented Sep 20, 2019

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvfangmin left a comment

Choose a reason for hiding this comment

hanm commented Sep 24, 2019

arshadmohammad commented Sep 26, 2019

arshadmohammad Sep 5, 2019 •

edited

hanm left a comment •

edited