ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-472

Making DataNode not instantiate a HashMap when the node is ephmeral

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.1.1, 3.2.0
    • Fix Version/s: 3.3.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Looking at the code, there is an overhead of a HashSet object for that nodes children, even though the node might be an ephmeral node and cannot have children.

      1. zookeeper-472.patch
        47 kB
        Erik Holstad
      2. zookeeper-472.patch
        48 kB
        Patrick Hunt
      3. zookeeper-472.patch
        46 kB
        Erik Holstad
      4. zookeeper-472.patch
        50 kB
        Erik Holstad
      5. zookeeper-472.patch
        49 kB
        Erik Holstad
      6. zookeeper-472.patch
        77 kB
        Erik Holstad

        Activity

        Hide
        Patrick Hunt added a comment -

        Sounds good - as long as we can make the change in a b/w compatible way w/o negatively impacting performance.

        Show
        Patrick Hunt added a comment - Sounds good - as long as we can make the change in a b/w compatible way w/o negatively impacting performance.
        Hide
        Benjamin Reed added a comment -

        i think we should expand this to not instantiate a hashmap for all zondes if there aren't any children. it creates a fixed size overhead for all leaf nodes and since there will always be more leaves than inner nodes, it is a none trivial space saving. i think it could also speed serialization/deserialization since it is faster to process a null then an empty hashmap. plus i think it keeps the code simpler to not have a new class.

        Show
        Benjamin Reed added a comment - i think we should expand this to not instantiate a hashmap for all zondes if there aren't any children. it creates a fixed size overhead for all leaf nodes and since there will always be more leaves than inner nodes, it is a none trivial space saving. i think it could also speed serialization/deserialization since it is faster to process a null then an empty hashmap. plus i think it keeps the code simpler to not have a new class.
        Hide
        Patrick Hunt added a comment -

        good point ben, +1
        good catch Erik, this is shaping up to be a decent savings.

        Show
        Patrick Hunt added a comment - good point ben, +1 good catch Erik, this is shaping up to be a decent savings.
        Hide
        Erik Holstad added a comment -

        After the HBase meeting this weekend we might have a use case for a new node type which keeps the children sorted, so including that we would have 3 different types of nodes, if this new node would be interesting for ZooKeeper in general. Tying that to this issue I came up with the following:
        1. Make a Node interface that only have the following methods
        copyStat(...)
        deserialize(...)
        serialize(...)
        2. For the ephemeral nodes this would be it, they wouldn't have to have any notion about children.
        3, For the regular nodes and the sorted ones would also have:
        setChildren(...)
        addChild(...)
        getChildren(...)
        where getChildren(...) could have different return types depending on the node.

        4. For the nodes that can have children we also keep the structure uninstantiated until children are actually set or added to the parent. This part I have already done, jsut need to do some more testing on it.

        Erik

        Show
        Erik Holstad added a comment - After the HBase meeting this weekend we might have a use case for a new node type which keeps the children sorted, so including that we would have 3 different types of nodes, if this new node would be interesting for ZooKeeper in general. Tying that to this issue I came up with the following: 1. Make a Node interface that only have the following methods copyStat(...) deserialize(...) serialize(...) 2. For the ephemeral nodes this would be it, they wouldn't have to have any notion about children. 3, For the regular nodes and the sorted ones would also have: setChildren(...) addChild(...) getChildren(...) where getChildren(...) could have different return types depending on the node. 4. For the nodes that can have children we also keep the structure uninstantiated until children are actually set or added to the parent. This part I have already done, jsut need to do some more testing on it. Erik
        Hide
        Erik Holstad added a comment -

        Did some more coding today and created 3 classes and 1 inteface:
        Interface Node

        Class EphemeralNode
        Class PersistentNode
        Class PersistentSortedNode

        The problem is that a lot of the class variables from the old DataNode class are being used like node.data = {}, instead of getting and setting it, which leads to some weirdness
        when the node is no longer a class, but an interface. So the way I see it we can take two approaches.
        1. Having the nodes subclass and override methods from a base class or
        2. Start using getters and setters for all variables in the nodes.

        Just want to check before I do anything more since this might turn out to be a quite big change of how the nodes are being used internally.

        Erik

        Show
        Erik Holstad added a comment - Did some more coding today and created 3 classes and 1 inteface: Interface Node Class EphemeralNode Class PersistentNode Class PersistentSortedNode The problem is that a lot of the class variables from the old DataNode class are being used like node.data = {}, instead of getting and setting it, which leads to some weirdness when the node is no longer a class, but an interface. So the way I see it we can take two approaches. 1. Having the nodes subclass and override methods from a base class or 2. Start using getters and setters for all variables in the nodes. Just want to check before I do anything more since this might turn out to be a quite big change of how the nodes are being used internally. Erik
        Hide
        Mahadev konar added a comment -

        I htought the jira was just about not instantiating hashmap.. no? Why does PersistentNode and Ephemeral node need to be different? They can be the same but with the property that until and unless a addchild is done, the hashset maps to null... wouldnt that be sufficient?

        Even with the above you wont be able to do sorted nodes since the chidlren are still sets... no? The children member should map to some other datastructure that maintains order.. no?

        Show
        Mahadev konar added a comment - I htought the jira was just about not instantiating hashmap.. no? Why does PersistentNode and Ephemeral node need to be different? They can be the same but with the property that until and unless a addchild is done, the hashset maps to null... wouldnt that be sufficient? Even with the above you wont be able to do sorted nodes since the chidlren are still sets... no? The children member should map to some other datastructure that maintains order.. no?
        Hide
        Erik Holstad added a comment -

        @Mahadev
        Yes the Jira was originally about not instantiating the HashSet before any children where added to the node, so in that case there is only a small reason for not having the persistent and the ephemeral nodes the same. For the sorted node I'm just using a sortedSet instead of the hashset, so all the set methods are still valid to use.

        Not really sure how many types of nodes there could be, but we have a use case for HBase where this new sorted node would be good.
        And in the case of more nodes coming into play it might make sense to have an interface of a base class.

        Sorry about not being clear.

        Erik

        Show
        Erik Holstad added a comment - @Mahadev Yes the Jira was originally about not instantiating the HashSet before any children where added to the node, so in that case there is only a small reason for not having the persistent and the ephemeral nodes the same. For the sorted node I'm just using a sortedSet instead of the hashset, so all the set methods are still valid to use. Not really sure how many types of nodes there could be, but we have a use case for HBase where this new sorted node would be good. And in the case of more nodes coming into play it might make sense to have an interface of a base class. Sorry about not being clear. Erik
        Hide
        Erik Holstad added a comment -

        I decided to take a lesser approach and just do what this Jira originally said, so basically creating two new methods, addChild() and removeChild() and making the variable children in DataNode private. The children set in DataNode is not instantiated at node creation but instead the first time addChild() is being called.

        No synchronization is done one the calls addChild and removeChild since it looks like most other calls are synchronized on the node itself.
        Not really sure why getChildren is synchronized though, since as far as I can see it all the uses of it are also synchronized on the node.

        According to the docs it says that 2 spaces should be used instead of 4. Is that a miss in the docs or is that what is really being used?
        I'm using 2 for this patch.

        Show
        Erik Holstad added a comment - I decided to take a lesser approach and just do what this Jira originally said, so basically creating two new methods, addChild() and removeChild() and making the variable children in DataNode private. The children set in DataNode is not instantiated at node creation but instead the first time addChild() is being called. No synchronization is done one the calls addChild and removeChild since it looks like most other calls are synchronized on the node itself. Not really sure why getChildren is synchronized though, since as far as I can see it all the uses of it are also synchronized on the node. According to the docs it says that 2 spaces should be used instead of 4. Is that a miss in the docs or is that what is really being used? I'm using 2 for this patch.
        Hide
        Patrick Hunt added a comment -

        Hi, what's the status on this JIRA? I'd like to get it reviewed/committed if it's ready. (you need to "submit patch", I would do it, but I can't as you've marked it as "in progress")

        we use 4 spaces, for some reason hadoop mr/hdfs uses 2 but we use 4 (you should see this used throughout our code)

        Thanks!

        Show
        Patrick Hunt added a comment - Hi, what's the status on this JIRA? I'd like to get it reviewed/committed if it's ready. (you need to "submit patch", I would do it, but I can't as you've marked it as "in progress") we use 4 spaces, for some reason hadoop mr/hdfs uses 2 but we use 4 (you should see this used throughout our code) Thanks!
        Hide
        Erik Holstad added a comment -

        @ Patrik Hunt, been waiting from someone to have a look at the patch for a little bit. The reason that I didn't mark it as patch available, was because
        I got yelled at in the past, not here, for marking it like that when it hadn't been reviewed Will change the formatting to 4 spaces and upload again.

        Erik

        Show
        Erik Holstad added a comment - @ Patrik Hunt, been waiting from someone to have a look at the patch for a little bit. The reason that I didn't mark it as patch available, was because I got yelled at in the past, not here, for marking it like that when it hadn't been reviewed Will change the formatting to 4 spaces and upload again. Erik
        Hide
        Patrick Hunt added a comment -

        No yelling here, maybe a stern talking to though.

        Update the patch and we'll take a look. If you release the "in progress" that will allow us to submit for you if we think it's ready.

        Show
        Patrick Hunt added a comment - No yelling here, maybe a stern talking to though. Update the patch and we'll take a look. If you release the "in progress" that will allow us to submit for you if we think it's ready.
        Hide
        Erik Holstad added a comment -

        Second version with some minor indention fixes.

        Show
        Erik Holstad added a comment - Second version with some minor indention fixes.
        Hide
        Erik Holstad added a comment -

        @ Patrik , well good thing you cannot take me in the ear and sit me down Might want to change the instructions on http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute though, to not make people confused? Remember taking a look there when I started and saw the code.

        Erik

        Show
        Erik Holstad added a comment - @ Patrik , well good thing you cannot take me in the ear and sit me down Might want to change the instructions on http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute though, to not make people confused? Remember taking a look there when I started and saw the code. Erik
        Hide
        Patrick Hunt added a comment -

        thanks for the pointer - I fixed the indentation item and also added note on notabs. also fixed a problem that should have said tests should end in Test, not start.

        Show
        Patrick Hunt added a comment - thanks for the pointer - I fixed the indentation item and also added note on notabs. also fixed a problem that should have said tests should end in Test, not start.
        Hide
        Patrick Hunt added a comment -

        Hi Erik, I tried patching the current trunk, but the patch fails (below). Perhaps it's against an older version of zk source? Would you mind
        creating a patch against the current source HEAD in svn? (I took a look but it seemed more than just a simple mismatch, easiest if you could address)

        Also it's best to have a single patch file as attachment, all you need to do is upload a file with the same name and jira will take care of it:
        from howtocontribute:
        "In some cases a patch may need to be updated based on review comments. In this case the updated patch should be re-attached to the Jira with the same name. Jira will archive the older version of the patch and make the new patch the active patch. This will enable a history of patches on the Jira. As stated above patch naming is generally ZOOKEEPER-#.patch where ZOOKEEPER-# is the id of the Jira."
        you could use "manage attachments" to address (ie remove the v2.patch)

        -------------------
        patch -p0 < /home/phunt/Desktop/zookeeper-472-v2.patch

        patching file src/java/main/org/apache/zookeeper/server/DataNode.java
        patching file src/java/main/org/apache/zookeeper/server/DataTree.java
        Hunk #35 FAILED at 1029.
        1 out of 35 hunks FAILED – saving rejects to file src/java/main/org/apache/zookeeper/server/DataTree.java.rej
        patching file src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
        Hunk #6 FAILED at 208.
        1 out of 9 hunks FAILED – saving rejects to file src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java.rej

        Show
        Patrick Hunt added a comment - Hi Erik, I tried patching the current trunk, but the patch fails (below). Perhaps it's against an older version of zk source? Would you mind creating a patch against the current source HEAD in svn? (I took a look but it seemed more than just a simple mismatch, easiest if you could address) Also it's best to have a single patch file as attachment, all you need to do is upload a file with the same name and jira will take care of it: from howtocontribute: "In some cases a patch may need to be updated based on review comments. In this case the updated patch should be re-attached to the Jira with the same name. Jira will archive the older version of the patch and make the new patch the active patch. This will enable a history of patches on the Jira. As stated above patch naming is generally ZOOKEEPER-#.patch where ZOOKEEPER-# is the id of the Jira." you could use "manage attachments" to address (ie remove the v2.patch) ------------------- patch -p0 < /home/phunt/Desktop/zookeeper-472-v2.patch patching file src/java/main/org/apache/zookeeper/server/DataNode.java patching file src/java/main/org/apache/zookeeper/server/DataTree.java Hunk #35 FAILED at 1029. 1 out of 35 hunks FAILED – saving rejects to file src/java/main/org/apache/zookeeper/server/DataTree.java.rej patching file src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java Hunk #6 FAILED at 208. 1 out of 9 hunks FAILED – saving rejects to file src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java.rej
        Hide
        Erik Holstad added a comment -

        Fixed issues, so that patch applies to trunk

        Show
        Erik Holstad added a comment - Fixed issues, so that patch applies to trunk
        Hide
        Erik Holstad added a comment -

        @ Patrik, sorry for not doing this yesterday, had some other stuff that I needed to take care of. Yeah, that patch was created against an older branch, but I updated to trunk
        in Eclipse and tested it before submitting it, but there seems to have been some issues with that.
        This patch applied clean on trunk as of this morning using regular patch -p0

        Erik

        Show
        Erik Holstad added a comment - @ Patrik, sorry for not doing this yesterday, had some other stuff that I needed to take care of. Yeah, that patch was created against an older branch, but I updated to trunk in Eclipse and tested it before submitting it, but there seems to have been some issues with that. This patch applied clean on trunk as of this morning using regular patch -p0 Erik
        Hide
        Mahadev konar added a comment -

        erik,
        I just tried compiling the code with ant jar and it fails to compile with the folowing error -

        compile:
            [javac] Compiling 104 source files to /home/mahadev/workspace/zookeeper-commit-trunk/build/classes
            [javac] /home/mahadev/workspace/zookeeper-commit-trunk/src/java/main/org/apache/zookeeper/server/DataTree.java:1055: children has private access in org.apache.zookeeper.server.DataNode
            [javac]                 node.parent.children.add(path.substring(lastSlash + 1));
            [javac]                            ^
            [javac] 1 error
        

        I havent ahd a chance to review the code yet, but I couldnt help but notice changes to license headers on src/java/main/org/apache/zookeeper/server/DataNode.java. Is there something you changed in the licence?

        Show
        Mahadev konar added a comment - erik, I just tried compiling the code with ant jar and it fails to compile with the folowing error - compile: [javac] Compiling 104 source files to /home/mahadev/workspace/zookeeper-commit-trunk/build/classes [javac] /home/mahadev/workspace/zookeeper-commit-trunk/src/java/main/org/apache/zookeeper/server/DataTree.java:1055: children has private access in org.apache.zookeeper.server.DataNode [javac] node.parent.children.add(path.substring(lastSlash + 1)); [javac] ^ [javac] 1 error I havent ahd a chance to review the code yet, but I couldnt help but notice changes to license headers on src/java/main/org/apache/zookeeper/server/DataNode.java. Is there something you changed in the licence?
        Hide
        Erik Holstad added a comment -

        Fixed compile error.

        Show
        Erik Holstad added a comment - Fixed compile error.
        Hide
        Erik Holstad added a comment -

        @ Mahadev, yeah that was my bad. Not sure why my environment didn't show that as an error.

        Erik

        Show
        Erik Holstad added a comment - @ Mahadev, yeah that was my bad. Not sure why my environment didn't show that as an error. Erik
        Hide
        Mahadev konar added a comment -

        erik, there are apache header changes in 2 files, any reason for that?

        Show
        Mahadev konar added a comment - erik, there are apache header changes in 2 files, any reason for that?
        Hide
        Erik Holstad added a comment -

        Fixed headers

        Show
        Erik Holstad added a comment - Fixed headers
        Hide
        Erik Holstad added a comment -

        @ Mahadev, no reason for it, just a part of the mess with me using another indention scheme for the original patch.

        Show
        Erik Holstad added a comment - @ Mahadev, no reason for it, just a part of the mess with me using another indention scheme for the original patch.
        Hide
        Patrick Hunt added a comment -

        Is this ready? I see some updates, throwing into the patch queue, reviewer please be sure all the comments are addressed.

        Show
        Patrick Hunt added a comment - Is this ready? I see some updates, throwing into the patch queue, reviewer please be sure all the comments are addressed.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12422389/zookeeper-472.patch
        against trunk revision 833938.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/60/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12422389/zookeeper-472.patch against trunk revision 833938. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/60/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Updated patch to compile against latest trunk.

        Also cleaned up some finals.

        Also reduced the default child hashset size to 8 rather than 16 (let's be conservative
        as to the avg number of subnodes).

        Small optimization to getchildren in datatree - allocate exactly the right size array list

        Show
        Patrick Hunt added a comment - Updated patch to compile against latest trunk. Also cleaned up some finals. Also reduced the default child hashset size to 8 rather than 16 (let's be conservative as to the avg number of subnodes). Small optimization to getchildren in datatree - allocate exactly the right size array list
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12424615/zookeeper-472.patch
        against trunk revision 833938.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/61/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/61/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/61/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12424615/zookeeper-472.patch against trunk revision 833938. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/61/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/61/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/61/console This message is automatically generated.
        Hide
        Erik Holstad added a comment -

        Fixed the findbug warning, hopefully, by moving the synchronization away from the node object into the different caller methods.

        Show
        Erik Holstad added a comment - Fixed the findbug warning, hopefully, by moving the synchronization away from the node object into the different caller methods.
        Hide
        Erik Holstad added a comment -

        Not really sure what to do about the no test -1. It is kinda hard to include new test for something like this and make it portable in a good way.
        Since I'm not adding any new functionality, the old tests should be enough, or what is the general opinion on this matter?

        Erik

        Show
        Erik Holstad added a comment - Not really sure what to do about the no test -1. It is kinda hard to include new test for something like this and make it portable in a good way. Since I'm not adding any new functionality, the old tests should be enough, or what is the general opinion on this matter? Erik
        Hide
        Patrick Hunt added a comment -

        In general we should have tests but I'm fine with the no-test in this case, this is an optimization not a bug fix
        and I can't think of any test we could add that would benefit, we already have good test coverage on this area.

        Show
        Patrick Hunt added a comment - In general we should have tests but I'm fine with the no-test in this case, this is an optimization not a bug fix and I can't think of any test we could add that would benefit, we already have good test coverage on this area.
        Hide
        Erik Holstad added a comment -

        Sounds good to me.

        Show
        Erik Holstad added a comment - Sounds good to me.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12424625/zookeeper-472.patch
        against trunk revision 833938.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/62/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/62/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/62/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12424625/zookeeper-472.patch against trunk revision 833938. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/62/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/62/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/62/console This message is automatically generated.
        Hide
        Mahadev konar added a comment -

        +1 .. the patch looks good. one minor nit for future patches, it becomes hard to review the patches if it has a lot of indentation changes. This patch had a lot of indentation than code changes and it became quite hard to review. Its always good to file indentation changes in a seperate jira so that reviewer can easily find out what actual code changes happened.

        Show
        Mahadev konar added a comment - +1 .. the patch looks good. one minor nit for future patches, it becomes hard to review the patches if it has a lot of indentation changes. This patch had a lot of indentation than code changes and it became quite hard to review. Its always good to file indentation changes in a seperate jira so that reviewer can easily find out what actual code changes happened.
        Hide
        Mahadev konar added a comment -

        I just committed this. thanks erik.

        Show
        Mahadev konar added a comment - I just committed this. thanks erik.
        Hide
        Erik Holstad added a comment -

        @Mahadev
        I totally agree that it is hard to follow a patch when there is a lot of "crap" changes in there. This was not my intention but had to do with the settings
        that I used, from the beginning and some un clearness on the website about intentations that has since been fixed.

        Show
        Erik Holstad added a comment - @Mahadev I totally agree that it is hard to follow a patch when there is a lot of "crap" changes in there. This was not my intention but had to do with the settings that I used, from the beginning and some un clearness on the website about intentations that has since been fixed.
        Hide
        Mahadev konar added a comment -

        no worries erik!

        Show
        Mahadev konar added a comment - no worries erik!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #536 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/536/)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #536 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/536/ )

          People

          • Assignee:
            Erik Holstad
            Reporter:
            Erik Holstad
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development