ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-555

Add stat information to GetChildrenResponse

    Details

    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      This change is backward incompatible with server versions prior to 3.3.0 given that the wire protocol has changed. However at the client API level this is a b/w compatible change (added new api methods).

      Changes required to have Stat returned with get_children().
      Show
      This change is backward incompatible with server versions prior to 3.3.0 given that the wire protocol has changed. However at the client API level this is a b/w compatible change (added new api methods). Changes required to have Stat returned with get_children().

      Description

      GetChildren() is the only non-create/delete API which does not include the node stat information. I propose that the definition of GetChildren() should be:

      class GetChildrenResponse

      { vector<ustring> children; org.apache.zookeeper.data.Stat stat; }

      There is a trivial fix to the server (FinalRequestProcessor.java): rsp = new GetChildrenResponse(children, stat);

      And something similar to the client library.

      1. ZOOKEEPER-555.patch
        66 kB
        Árni Már Jónsson
      2. ZOOKEEPER-555.patch
        65 kB
        Patrick Hunt
      3. ZOOKEEPER-555.patch
        70 kB
        Patrick Hunt
      4. getchildren_stat.patch
        11 kB
        Árni Már Jónsson

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Patch Available Patch Available Open Open
          11h 43m 4 Patrick Hunt 21/Oct/09 04:41
          Open Open Patch Available Patch Available
          5d 4h 59m 5 Patrick Hunt 21/Oct/09 04:42
          Patch Available Patch Available Resolved Resolved
          9d 19h 27m 1 Mahadev konar 30/Oct/09 23:09
          Resolved Resolved Closed Closed
          146d 18h 15m 1 Patrick Hunt 26/Mar/10 17:25
          Patrick Hunt made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Patrick Hunt made changes -
          Hadoop Flags [Reviewed] [Incompatible change, Reviewed]
          Release Note Changes required to have Stat returned with get_children(). This change is backward incompatible with server versions prior to 3.3.0 given that the wire protocol has changed. However at the client API level this is a b/w compatible change (added new api methods).

          Changes required to have Stat returned with get_children().
          Patrick Hunt made changes -
          Link This issue is related to ZOOKEEPER-567 [ ZOOKEEPER-567 ]
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #514 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/514/)
          . Add stat information to GetChildrenResponse. (Arni Jonson and phunt via mahadev)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #514 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/514/ ) . Add stat information to GetChildrenResponse. (Arni Jonson and phunt via mahadev)
          Mahadev konar made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Mahadev konar added a comment -

          I just commmited this. thanks arni and pat.

          Show
          Mahadev konar added a comment - I just commmited this. thanks arni and pat.
          Hide
          Mahadev konar added a comment -

          +1 this looks good...

          Show
          Mahadev konar added a comment - +1 this looks good...
          Hide
          Mahadev konar added a comment -

          +1 this looks good...

          Show
          Mahadev konar added a comment - +1 this looks good...
          Hide
          Patrick Hunt added a comment -

          I believe the release audit failure is just due to adding a new public field to OpCode.

          Mahadev please review and commit this asap.

          Show
          Patrick Hunt added a comment - I believe the release audit failure is just due to adding a new public field to OpCode. Mahadev please review and commit this asap.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422753/ZOOKEEPER-555.patch
          against trunk revision 826787.

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

          +1 tests included. The patch appears to include 28 new or modified tests.

          +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 generated 182 release audit warnings (more than the trunk's current 181 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/31/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/31/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/31/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/31/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/12422753/ZOOKEEPER-555.patch against trunk revision 826787. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 28 new or modified tests. +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 generated 182 release audit warnings (more than the trunk's current 181 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/31/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/31/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/31/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/31/console This message is automatically generated.
          Patrick Hunt made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Patrick Hunt made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422753/ZOOKEEPER-555.patch
          against trunk revision 826787.

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

          +1 tests included. The patch appears to include 28 new or modified tests.

          +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 generated 182 release audit warnings (more than the trunk's current 181 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/30/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/30/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/30/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/30/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/12422753/ZOOKEEPER-555.patch against trunk revision 826787. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 28 new or modified tests. +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 generated 182 release audit warnings (more than the trunk's current 181 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/30/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/30/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/30/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/30/console This message is automatically generated.
          Patrick Hunt made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Patrick Hunt made changes -
          Attachment ZOOKEEPER-555.patch [ 12422753 ]
          Hide
          Patrick Hunt added a comment -

          Same patch as previous except that I removed an unnecessary stat copy from original getChildren in finalrequestprocessor

          Show
          Patrick Hunt added a comment - Same patch as previous except that I removed an unnecessary stat copy from original getChildren in finalrequestprocessor
          Patrick Hunt made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422748/ZOOKEEPER-555.patch
          against trunk revision 826787.

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

          +1 tests included. The patch appears to include 28 new or modified tests.

          +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/29/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/29/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/29/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/12422748/ZOOKEEPER-555.patch against trunk revision 826787. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 28 new or modified tests. +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/29/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/29/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/29/console This message is automatically generated.
          Patrick Hunt made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Patrick Hunt made changes -
          Attachment ZOOKEEPER-555.patch [ 12422748 ]
          Hide
          Patrick Hunt added a comment -

          This is Arni's patch with the following changes:

          1) resolved findbugs issues (on my system at least)
          2) fixed the "ls2" command in the Java CLI
          3) minor changes to the c/java api docs
          4) changed ZooDefs.OpCode.getChildren1 back to getChildren as this is a public
          interface (granted it shouldn't be used by outside code but it is public, safer to keep
          it the same name)

          5) I also removed the StringsAndStat jute type. Not that it was bad,
          but it wasn't used in the c code and it's not consistent with the current Java api (getData for
          example). As part of this I removed getChildren2 and overloaded getChildren with Stat param.
          Better? Worse? Comments?

          ps. I verified that filling in the stat and the child list is done under a node lock (would be good for
          any other reviewer to also check my check).

          Show
          Patrick Hunt added a comment - This is Arni's patch with the following changes: 1) resolved findbugs issues (on my system at least) 2) fixed the "ls2" command in the Java CLI 3) minor changes to the c/java api docs 4) changed ZooDefs.OpCode.getChildren1 back to getChildren as this is a public interface (granted it shouldn't be used by outside code but it is public, safer to keep it the same name) 5) I also removed the StringsAndStat jute type. Not that it was bad, but it wasn't used in the c code and it's not consistent with the current Java api (getData for example). As part of this I removed getChildren2 and overloaded getChildren with Stat param. Better? Worse? Comments? ps. I verified that filling in the stat and the child list is done under a node lock (would be good for any other reviewer to also check my check).
          Patrick Hunt made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Patrick Hunt added a comment -

          Nice, thanks. I'll review and look at the hadoopqa failure and will attempt to address it myself.

          Show
          Patrick Hunt added a comment - Nice, thanks. I'll review and look at the hadoopqa failure and will attempt to address it myself.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422675/ZOOKEEPER-555.patch
          against trunk revision 826787.

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

          +1 tests included. The patch appears to include 29 new or modified tests.

          +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 generated 183 release audit warnings (more than the trunk's current 179 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/28/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/28/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/28/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/28/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/12422675/ZOOKEEPER-555.patch against trunk revision 826787. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 29 new or modified tests. +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 generated 183 release audit warnings (more than the trunk's current 179 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/28/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/28/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/28/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/28/console This message is automatically generated.
          Árni Már Jónsson made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hadoop Flags [Incompatible change]
          Árni Már Jónsson made changes -
          Attachment ZOOKEEPER-555.patch [ 12422675 ]
          Hide
          Árni Már Jónsson added a comment -

          A patch which adds a second getChildren() function, which in addition to children, returns the parent stat.

          The main additions are these:

          • There is a new C callback, strings_stat_completion_t
          • There are 4 new C API calls: zoo_aget_children2(), zoo_awget_children(), zoo_get_children2(), zoo_wget_children2()
          • The C client now supports the "ls2" command
          • The C test client has a new test, testGetChildren2()
          • There is a new Java API callback: Children2Callback()
          • There are 2 new message: GetChildren2Request(), GetChildren2Response()
          • There is a new struct> StringsAndStat, which is the unit of data transfer in the C/Java API.
          • There are 4 new Java API calls
          • There are new tests, and test modifications to the Java unit-tests. The modifications are corresponding calls to getChildren2() alongside a getChildren() call, the return values of which are then compared. There is a new class under the AsyncOps tests, which is mostly equivalent to the getChildren()-calling class. And there is a new test> GetChildren2Test.java, which is very similar to the exists() test.
          Show
          Árni Már Jónsson added a comment - A patch which adds a second getChildren() function, which in addition to children, returns the parent stat. The main additions are these: There is a new C callback, strings_stat_completion_t There are 4 new C API calls: zoo_aget_children2(), zoo_awget_children(), zoo_get_children2(), zoo_wget_children2() The C client now supports the "ls2" command The C test client has a new test, testGetChildren2() There is a new Java API callback: Children2Callback() There are 2 new message: GetChildren2Request(), GetChildren2Response() There is a new struct> StringsAndStat, which is the unit of data transfer in the C/Java API. There are 4 new Java API calls There are new tests, and test modifications to the Java unit-tests. The modifications are corresponding calls to getChildren2() alongside a getChildren() call, the return values of which are then compared. There is a new class under the AsyncOps tests, which is mostly equivalent to the getChildren()-calling class. And there is a new test> GetChildren2Test.java, which is very similar to the exists() test.
          Hide
          Mahadev konar added a comment -

          agreed. I do think the Stat information with getChidren info provides more and useful information which can be used for total ordering of getChildren requests!

          Show
          Mahadev konar added a comment - agreed. I do think the Stat information with getChidren info provides more and useful information which can be used for total ordering of getChildren requests!
          Hide
          Patrick Hunt added a comment -

          I continue to be impressed by the power of just the few primitives/guarantees that we provide.

          That said,I consider my solution a bit of a hack (not necessarily bad, just not optimal), really
          Arni's solution is the correct one, we should provide the ability to get the stat as part of the
          get_children. However this (stat) won't be generally available till 3.3 even assuming Arni completes
          it (please!) and this other alternative might be a good enough solution in the mean time.

          Show
          Patrick Hunt added a comment - I continue to be impressed by the power of just the few primitives/guarantees that we provide. That said,I consider my solution a bit of a hack (not necessarily bad, just not optimal), really Arni's solution is the correct one, we should provide the ability to get the stat as part of the get_children. However this (stat) won't be generally available till 3.3 even assuming Arni completes it (please!) and this other alternative might be a good enough solution in the mean time.
          Hide
          Mahadev konar added a comment -

          was just reading through this ... very clever solution pat! ....

          Show
          Mahadev konar added a comment - was just reading through this ... very clever solution pat! ....
          Hide
          Árni Már Jónsson added a comment -

          That's clever. The 2-tuple (y, -x) would give a total version order. Cool things those sequence nodes.

          I'll see tomorrow if I'm still motivated to finish the patch.

          Thanks.

          Show
          Árni Már Jónsson added a comment - That's clever. The 2-tuple (y, -x) would give a total version order. Cool things those sequence nodes. I'll see tomorrow if I'm still motivated to finish the patch. Thanks.
          Hide
          Patrick Hunt added a comment -

          That sounds reasonable.

          Alternately, might you use the sequence numbers themselves to ascertain the "version"?

          I was thinking something like: you have a child list with seq numbers

          version = x,y where x is count, y is last seq number

          1 2 3 => version is 3,3
          znode 2 drops
          1 3 => version is 2,3
          a new znode is added
          1 3 4 => version is 3,4

          so the server would see 3,3 2,3 3,3 2,3 in your example

          x,y where x can only decrease as time increases unless y increases. therefore the second 3,3 2,3 would be ignored (ie old version)

          Show
          Patrick Hunt added a comment - That sounds reasonable. Alternately, might you use the sequence numbers themselves to ascertain the "version"? I was thinking something like: you have a child list with seq numbers version = x,y where x is count, y is last seq number 1 2 3 => version is 3,3 znode 2 drops 1 3 => version is 2,3 a new znode is added 1 3 4 => version is 3,4 so the server would see 3,3 2,3 3,3 2,3 in your example x,y where x can only decrease as time increases unless y increases. therefore the second 3,3 2,3 would be ignored (ie old version)
          Hide
          Árni Már Jónsson added a comment -

          Hi,

          Thank you for the advice.

          The use case I'm interested involves leader election. I have a set servers, 1 master and multiple slaves. Each server publishes an ephemeral sequence child znode into a commonly known znode, of the form host:port. The first node (sorted on the sequence number) is considered to be the master. The servers only publish their existence, and are generally not aware of each other.

          Clients are responsible for notifying the servers if they are master, or if they are slaves, and if so who is the master. They do this when they first read the child list, and each time the child list changes. The notifications are idempotent, but if e.g. the list changes 2 times, and one client is able to notify the servers of both version before some other client sees them, the servers will have their status changes 4 times instead of 2. Worst case is N x #clients, for N changes, instead of N.

          I'd like to keep the cost of these as small as possible. One way is to include the child list version number with the notification, and have the server only accept it, if the version is larger than one seen before.

          I'll prepare a new patch which fits your requirements.

          Show
          Árni Már Jónsson added a comment - Hi, Thank you for the advice. The use case I'm interested involves leader election. I have a set servers, 1 master and multiple slaves. Each server publishes an ephemeral sequence child znode into a commonly known znode, of the form host:port. The first node (sorted on the sequence number) is considered to be the master. The servers only publish their existence, and are generally not aware of each other. Clients are responsible for notifying the servers if they are master, or if they are slaves, and if so who is the master. They do this when they first read the child list, and each time the child list changes. The notifications are idempotent, but if e.g. the list changes 2 times, and one client is able to notify the servers of both version before some other client sees them, the servers will have their status changes 4 times instead of 2. Worst case is N x #clients, for N changes, instead of N. I'd like to keep the cost of these as small as possible. One way is to include the child list version number with the notification, and have the server only accept it, if the version is larger than one seen before. I'll prepare a new patch which fits your requirements.
          Patrick Hunt made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Assignee Árni Már Jónsson [ arnimarj ]
          Hide
          Patrick Hunt added a comment -

          I think this is a reasonable thing to add. Could you explain the use case where this is necessary?

          However there are some issues with the patch that need to be addressed:

          1) we require all changes in minor/fix releases to be b/w compatible and generally we try to do non-b/w compatible
          changes very infrequently. When you are running a highly available/reliable service you don't want to have to tell
          ppl "shutdown & upgrade all of your servers and clients when you upgrade to this version". The changes
          as currently implemented change both the API and wire protocol for existing functionality, this is not
          something we can accept.

          However, a simple workaround would be to add a new protocol (jute) type, and additional functions/callbacks
          to support getting children including the stat. Take a look at zoo_set2 in the c source, this is an example where
          simliar issue has happened before (although in that case jute had the stat, we just weren't providing it to
          the user as part of the api for zoo_set, so zoo_set2 was added). So *get_children2(... in the C case and
          probably just overload getChildren in java to have additional Stat param tacked onto the end.

          2) there need to be tests included in the patch that verify this functionality

          3) no tabs in the source, spaces only for indentation

          Note to reviewer: ensure that the implementation ensures that the version fields in the stat correspond to
          the child list returned (ie they are the same version, this is important if ppl start using the cversion
          correlated with the child list itself)

          Show
          Patrick Hunt added a comment - I think this is a reasonable thing to add. Could you explain the use case where this is necessary? However there are some issues with the patch that need to be addressed: 1) we require all changes in minor/fix releases to be b/w compatible and generally we try to do non-b/w compatible changes very infrequently. When you are running a highly available/reliable service you don't want to have to tell ppl "shutdown & upgrade all of your servers and clients when you upgrade to this version". The changes as currently implemented change both the API and wire protocol for existing functionality, this is not something we can accept. However, a simple workaround would be to add a new protocol (jute) type, and additional functions/callbacks to support getting children including the stat. Take a look at zoo_set2 in the c source, this is an example where simliar issue has happened before (although in that case jute had the stat, we just weren't providing it to the user as part of the api for zoo_set, so zoo_set2 was added). So *get_children2(... in the C case and probably just overload getChildren in java to have additional Stat param tacked onto the end. 2) there need to be tests included in the patch that verify this functionality 3) no tabs in the source, spaces only for indentation Note to reviewer : ensure that the implementation ensures that the version fields in the stat correspond to the child list returned (ie they are the same version, this is important if ppl start using the cversion correlated with the child list itself)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12422239/getchildren_stat.patch
          against trunk revision 824981.

          +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/27/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/12422239/getchildren_stat.patch against trunk revision 824981. +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/27/console This message is automatically generated.
          Árni Már Jónsson made changes -
          Attachment getchildren_stat.patch [ 12422239 ]
          Hide
          Árni Már Jónsson added a comment -

          getchildren stat patch

          Show
          Árni Már Jónsson added a comment - getchildren stat patch
          Árni Már Jónsson made changes -
          Field Original Value New Value
          Status Open [ 1 ] Patch Available [ 10002 ]
          Release Note Changes required to have Stat returned with get_children().
          Affects Version/s 3.3.0 [ 12313976 ]
          Fix Version/s 3.3.0 [ 12313976 ]
          Hide
          Árni Már Jónsson added a comment -

          Patch for server part (simple), and C client library (complicated).

          Tested this using the Python bindings (I have a separate patch for those if there is interest).

          Show
          Árni Már Jónsson added a comment - Patch for server part (simple), and C client library (complicated). Tested this using the Python bindings (I have a separate patch for those if there is interest).
          Árni Már Jónsson created issue -

            People

            • Assignee:
              Árni Már Jónsson
              Reporter:
              Árni Már Jónsson
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development