ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-631

zkpython's C code could do with a style clean-up

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.1, 3.4.0
    • Component/s: contrib-bindings
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Inconsistent formatting / use of parenthesis / some error checking - all need fixing.

      Also, the documentation in the header file could do with a reformat.

        Issue Links

          Activity

          Mahadev konar made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Patrick Hunt made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Patrick Hunt added a comment -

          Committed to 3.3 branch and trunk. Thanks Henry!

          I verified by inspecting the code changes, running the unit tests, but I also used the zk-smoketest/zk-latencies
          http://github.com/phunt/zk-smoketest to verify the changed code.

          One suggestion - next time create multiple JIRAs with individual patches rather than one big patch. Would
          make it much easier to review (say reformatting only, docs, acl/memory fixes. something like that.)

          Show
          Patrick Hunt added a comment - Committed to 3.3 branch and trunk. Thanks Henry! I verified by inspecting the code changes, running the unit tests, but I also used the zk-smoketest/zk-latencies http://github.com/phunt/zk-smoketest to verify the changed code. One suggestion - next time create multiple JIRAs with individual patches rather than one big patch. Would make it much easier to review (say reformatting only, docs, acl/memory fixes. something like that.)
          Patrick Hunt made changes -
          Link This issue blocks ZOOKEEPER-742 [ ZOOKEEPER-742 ]
          Henry Robinson made changes -
          Fix Version/s 3.4.0 [ 12314469 ]
          Henry Robinson made changes -
          Link This issue is related to ZOOKEEPER-740 [ ZOOKEEPER-740 ]
          Henry Robinson made changes -
          Fix Version/s 3.3.1 [ 12314846 ]
          Hide
          Henry Robinson added a comment -

          The existing tests are the ones that validate this patch. To test the Py_None and memory allocation issues is hard because in the first case the GC behaviour is hard to force and in the second we would have to stub out calloc(..) somehow!

          Show
          Henry Robinson added a comment - The existing tests are the ones that validate this patch. To test the Py_None and memory allocation issues is hard because in the first case the GC behaviour is hard to force and in the second we would have to stub out calloc(..) somehow!
          Hide
          Hadoop QA added a comment -

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

          +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-h1.grid.sp2.yahoo.net/57/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/57/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/57/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/12442027/ZOOKEEPER-631.patch against trunk revision 934312. +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-h1.grid.sp2.yahoo.net/57/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/57/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/57/console This message is automatically generated.
          Henry Robinson made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Henry Robinson made changes -
          Field Original Value New Value
          Attachment ZOOKEEPER-631.patch [ 12442027 ]
          Hide
          Henry Robinson added a comment -

          Attached patch addresses the following:

          1. Formatting redone to be (nearly) consistent
          2. Comments added to every function
          3. zookeeper.c reorganised logically
          4. Py_None now reference counted correctly (see ZOOKEEPER-742)
          5. Memory allocations now checked, and general error handling greatly improved.
          6. A variety of small bugs and typos fixed

          The result is hopefully a much more stable zkpython. This patch will look like a rewrite - there are lots of changes. Apologies to the reviewer in advance!

          (I am happy for this patch to be used by the ASF, but the button is not available to be checked).

          Show
          Henry Robinson added a comment - Attached patch addresses the following: 1. Formatting redone to be (nearly) consistent 2. Comments added to every function 3. zookeeper.c reorganised logically 4. Py_None now reference counted correctly (see ZOOKEEPER-742 ) 5. Memory allocations now checked, and general error handling greatly improved. 6. A variety of small bugs and typos fixed The result is hopefully a much more stable zkpython. This patch will look like a rewrite - there are lots of changes. Apologies to the reviewer in advance! (I am happy for this patch to be used by the ASF, but the button is not available to be checked).
          Henry Robinson created issue -

            People

            • Assignee:
              Henry Robinson
              Reporter:
              Henry Robinson
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development