Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.4.0
    • Component/s: contrib
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      A pub sub system using BooKkeeper and ZooKeeper with C++ and Java client bindings.

      Description

      we have developed a large scale pub/sub system based on ZooKeeper and BookKeeper.

      1. ZOOKEEPER-775.patch
        1.15 MB
        Mahadev konar
      2. ZOOKEEPER-775.patch
        1.40 MB
        Erwin Tam
      3. ZOOKEEPER-775.patch
        1.41 MB
        Erwin Tam
      4. libs_2.zip
        3.22 MB
        Benjamin Reed
      5. ZOOKEEPER-775_3.patch
        1013 kB
        Benjamin Reed
      6. libs.zip
        3.22 MB
        Benjamin Reed
      7. ZOOKEEPER-775_2.patch
        1008 kB
        Benjamin Reed
      8. ZOOKEEPER-775.patch
        996 kB
        Benjamin Reed
      There are no Sub-Tasks for this issue.

        Activity

        Hide
        Benjamin Reed added a comment -

        the tar file with the sources. we need to update the package names to be in the apache space. we also need to integrate it into the build. it uses maven.

        Show
        Benjamin Reed added a comment - the tar file with the sources. we need to update the package names to be in the apache space. we also need to integrate it into the build. it uses maven.
        Hide
        Benjamin Reed added a comment -

        the tar file with the sources. we need to update the package names to be in the apache space. we also need to integrate it into the build. it uses maven.

        Show
        Benjamin Reed added a comment - the tar file with the sources. we need to update the package names to be in the apache space. we also need to integrate it into the build. it uses maven.
        Hide
        Patrick Hunt added a comment -

        Ben you planning to make these changes or looking for help? (package/build), anything else that needs to be done that you know of?

        Show
        Patrick Hunt added a comment - Ben you planning to make these changes or looking for help? (package/build), anything else that needs to be done that you know of?
        Hide
        Benjamin Reed added a comment -

        sorry, i should have listed the todos:

        1) we need to agree on a package name. i propose org.apache.hedwig. i can fix the package references when we decide. (the package is currently in the com.yahoo namespace)

        2) we need to figure out where to put it. i'm assuming contrib/hedwig

        3) finally, we need to integrate into the build. this is a bit harder for me. we currently build hedwig with maven pulling zookeeper and bookkeeper from repositories. i'm not sure how to integrate into our current zookeeper build. so, if some one could take this last one, i would appreciate it.

        Show
        Benjamin Reed added a comment - sorry, i should have listed the todos: 1) we need to agree on a package name. i propose org.apache.hedwig. i can fix the package references when we decide. (the package is currently in the com.yahoo namespace) 2) we need to figure out where to put it. i'm assuming contrib/hedwig 3) finally, we need to integrate into the build. this is a bit harder for me. we currently build hedwig with maven pulling zookeeper and bookkeeper from repositories. i'm not sure how to integrate into our current zookeeper build. so, if some one could take this last one, i would appreciate it.
        Hide
        Patrick Hunt added a comment -

        1) we need to agree on a package name. i propose org.apache.hedwig. i can fix the package references when we decide. (the package is currently in the com.yahoo namespace)

        sounds good (similar to what we did with bk)

        2) we need to figure out where to put it. i'm assuming contrib/hedwig

        agree

        3) finally, we need to integrate into the build. this is a bit harder for me. we currently build hedwig with maven pulling zookeeper and bookkeeper from repositories. i'm not sure how to integrate into our current zookeeper build. so, if some one could take this last one, i would appreciate it.

        why don't we punt on this for now. just add a README.txt that explains that maven is used for the build. we can support maven for build/dev now and at some point add a simple build.xml that handles the packaging for the release. (if you agree add a JIRA for this task (build.xml) assigned to 3.4.0)

        Show
        Patrick Hunt added a comment - 1) we need to agree on a package name. i propose org.apache.hedwig. i can fix the package references when we decide. (the package is currently in the com.yahoo namespace) sounds good (similar to what we did with bk) 2) we need to figure out where to put it. i'm assuming contrib/hedwig agree 3) finally, we need to integrate into the build. this is a bit harder for me. we currently build hedwig with maven pulling zookeeper and bookkeeper from repositories. i'm not sure how to integrate into our current zookeeper build. so, if some one could take this last one, i would appreciate it. why don't we punt on this for now. just add a README.txt that explains that maven is used for the build. we can support maven for build/dev now and at some point add a simple build.xml that handles the packaging for the release. (if you agree add a JIRA for this task (build.xml) assigned to 3.4.0)
        Hide
        Benjamin Reed added a comment -

        this moves things to the right places and moves to the right packages, but maven is hating me! for some reason it isn't pulling the protocol buffers.

        Show
        Benjamin Reed added a comment - this moves things to the right places and moves to the right packages, but maven is hating me! for some reason it isn't pulling the protocol buffers.
        Hide
        Benjamin Reed added a comment -

        this should build now. i've attached a libs.zip that contains bookkeeper and zookeeper libraries. i suggest that we commit this ASAP and then do the work to fix the build once the code is in subversion.

        Show
        Benjamin Reed added a comment - this should build now. i've attached a libs.zip that contains bookkeeper and zookeeper libraries. i suggest that we commit this ASAP and then do the work to fix the build once the code is in subversion.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12445481/libs.zip
        against trunk revision 947063.

        +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-h1.grid.sp2.yahoo.net/106/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/12445481/libs.zip against trunk revision 947063. +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-h1.grid.sp2.yahoo.net/106/console This message is automatically generated.
        Hide
        Ivan Kelly added a comment -

        This patch doesn't include the ssl cert server.p12 as it's binary. This causes some tests to fail. Perhaps you should add it to libs.zip?

        Show
        Ivan Kelly added a comment - This patch doesn't include the ssl cert server.p12 as it's binary. This causes some tests to fail. Perhaps you should add it to libs.zip?
        Hide
        Benjamin Reed added a comment -

        added missing file indicated by ivan

        Show
        Benjamin Reed added a comment - added missing file indicated by ivan
        Hide
        Patrick Hunt added a comment -

        My initial feedback:

        The build fails with proto buff errors. I already have protobuf installed for other reasons, can't mvn pull down the protobuf version it needs vs relying on the system wide libs?

        Including the bk/zk jar files in server/lib is a really bad practice. Why do we have to do this vs using versions built from the current tree? If we have to do ths we should also indicate that this is not a release version - typically in mvn this means putting something like "SNAPSHOT" into the name of the jars.

        The README should have a tiny bit of text describing, at a very high level, what hedwig is.

        The contents of the scripts directory are not executable by default, we should either fix this in the patch or note it for committer to fix at commit time.

        ZooKeeperTestBase.java has windows eol characters. Perhaps run dos2unix on all the files and refresh the patch? (looks like all the tests suffer from this?)

        RAT identified the following files which seem like they should have license headers added:

        ./formatter.xml
        ./pom.xml
        ./client/pom.xml
        ./client/src/main/cpp/Makefile
        ./client/src/main/cpp/log4cpp.conf
        ./client/src/main/resources/log4j.properties
        ./doc/build.txt
        ./doc/dev.txt
        ./doc/doc.txt
        ./doc/user.txt
        ./protocol/Makefile
        ./protocol/pom.xml
        ./protocol/src/main/protobuf/PubSubProtocol.proto
        ./scripts/analyze.py
        ./scripts/hw.bash
        ./scripts/quote
        ./server/pom.xml

        Show
        Patrick Hunt added a comment - My initial feedback: The build fails with proto buff errors. I already have protobuf installed for other reasons, can't mvn pull down the protobuf version it needs vs relying on the system wide libs? Including the bk/zk jar files in server/lib is a really bad practice. Why do we have to do this vs using versions built from the current tree? If we have to do ths we should also indicate that this is not a release version - typically in mvn this means putting something like "SNAPSHOT" into the name of the jars. The README should have a tiny bit of text describing, at a very high level, what hedwig is. The contents of the scripts directory are not executable by default, we should either fix this in the patch or note it for committer to fix at commit time. ZooKeeperTestBase.java has windows eol characters. Perhaps run dos2unix on all the files and refresh the patch? (looks like all the tests suffer from this?) RAT identified the following files which seem like they should have license headers added: ./formatter.xml ./pom.xml ./client/pom.xml ./client/src/main/cpp/Makefile ./client/src/main/cpp/log4cpp.conf ./client/src/main/resources/log4j.properties ./doc/build.txt ./doc/dev.txt ./doc/doc.txt ./doc/user.txt ./protocol/Makefile ./protocol/pom.xml ./protocol/src/main/protobuf/PubSubProtocol.proto ./scripts/analyze.py ./scripts/hw.bash ./scripts/quote ./server/pom.xml
        Hide
        Patrick Hunt added a comment -

        Cancelling patch until issues are addressed.

        Show
        Patrick Hunt added a comment - Cancelling patch until issues are addressed.
        Hide
        Benjamin Reed added a comment -

        i would like to fix the build once we have it in the subversion repository.

        should i just remove the README? i'm not sure it is worth expanding since it would duplicate text in the docs directory

        i'll fix the scripts and the dos2unix

        with respect to the headers, i notice that configs, docs, and Makefiles don't have the license header in the zk repository, which leaves:

        ./pom.xml
        ./client/pom.xml
        ./protocol/pom.xml
        ./protocol/src/main/protobuf/PubSubProtocol.proto
        ./scripts/analyze.py
        ./scripts/hw.bash
        ./scripts/quote
        ./server/pom.xml

        is it okay if i just do those?

        Show
        Benjamin Reed added a comment - i would like to fix the build once we have it in the subversion repository. should i just remove the README? i'm not sure it is worth expanding since it would duplicate text in the docs directory i'll fix the scripts and the dos2unix with respect to the headers, i notice that configs, docs, and Makefiles don't have the license header in the zk repository, which leaves: ./pom.xml ./client/pom.xml ./protocol/pom.xml ./protocol/src/main/protobuf/PubSubProtocol.proto ./scripts/analyze.py ./scripts/hw.bash ./scripts/quote ./server/pom.xml is it okay if i just do those?
        Hide
        Patrick Hunt added a comment -

        i would like to fix the build once we have it in the subversion repository.

        by this I take it you mean removing the libs? Ok, but we should name the lib jars with SNAPSHOT as part of the initial commit to avoid any confusion (that's not a problem, right?)

        readme

        It's good to have the readme even if it's a bit short/duplicative. Provides a common hook for new users.

        licenses

        rat found all that, what you are suggesting is inline with core, so those exceptions sound fine.

        Show
        Patrick Hunt added a comment - i would like to fix the build once we have it in the subversion repository. by this I take it you mean removing the libs? Ok, but we should name the lib jars with SNAPSHOT as part of the initial commit to avoid any confusion (that's not a problem, right?) readme It's good to have the readme even if it's a bit short/duplicative. Provides a common hook for new users. licenses rat found all that, what you are suggesting is inline with core, so those exceptions sound fine.
        Hide
        Benjamin Reed added a comment -

        updated to address phunts comments.

        Show
        Benjamin Reed added a comment - updated to address phunts comments.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12446057/libs_2.zip
        against trunk revision 947063.

        +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-h1.grid.sp2.yahoo.net/113/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/12446057/libs_2.zip against trunk revision 947063. +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-h1.grid.sp2.yahoo.net/113/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        Much closer but I'm not able to run the hw.bash script successfully. When I run the commands listed in user.txt I get the following. Perhaps this was not updated for the lib renaming? I think after this is addressed, and I'm able to successfully run the example, we should be good to commit.

        scripts/hw.bash zk 2181 ./zkdata
        Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/zookeeper/server/quorum/QuorumPeerMain
        Caused by: java.lang.ClassNotFoundException: org.apache.zookeeper.server.quorum.QuorumPeerMain
        at java.net.URLClassLoader$1.run(URLClassLoader.java:202)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:190)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:307)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:248)
        Could not find the main class: org.apache.zookeeper.server.quorum.QuorumPeerMain. Program will exit.

        Show
        Patrick Hunt added a comment - Much closer but I'm not able to run the hw.bash script successfully. When I run the commands listed in user.txt I get the following. Perhaps this was not updated for the lib renaming? I think after this is addressed, and I'm able to successfully run the example, we should be good to commit. scripts/hw.bash zk 2181 ./zkdata Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/zookeeper/server/quorum/QuorumPeerMain Caused by: java.lang.ClassNotFoundException: org.apache.zookeeper.server.quorum.QuorumPeerMain at java.net.URLClassLoader$1.run(URLClassLoader.java:202) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:190) at java.lang.ClassLoader.loadClass(ClassLoader.java:307) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:301) at java.lang.ClassLoader.loadClass(ClassLoader.java:248) Could not find the main class: org.apache.zookeeper.server.quorum.QuorumPeerMain. Program will exit.
        Hide
        Erwin Tam added a comment -

        Latest Hedwig apache branch code along with some simplified Hedwig server hub start up scripts.

        Show
        Erwin Tam added a comment - Latest Hedwig apache branch code along with some simplified Hedwig server hub start up scripts.
        Hide
        Erwin Tam added a comment -

        Latest version of the Hedwig code in the apache branch. This also includes the C++ client bindings that Ivan Kelly worked on. Looking at past comments, if you are using the hw.bash script to startup ZK, BK, or Hedwig hubs, this is intended to be done on remote boxes. So you'll need to build/compile to get the Hedwig fat server jar and prepend the "push_jar=true" command to the hw.bash line so it'll copy over the jar to the remote server(s) and place it in the appropriate location. The CLASSPATH remotely is set to point to this area and won't work without it.

        Show
        Erwin Tam added a comment - Latest version of the Hedwig code in the apache branch. This also includes the C++ client bindings that Ivan Kelly worked on. Looking at past comments, if you are using the hw.bash script to startup ZK, BK, or Hedwig hubs, this is intended to be done on remote boxes. So you'll need to build/compile to get the Hedwig fat server jar and prepend the "push_jar=true" command to the hw.bash line so it'll copy over the jar to the remote server(s) and place it in the appropriate location. The CLASSPATH remotely is set to point to this area and won't work without it.
        Hide
        Mahadev konar added a comment -

        erwin/ben,
        Would it be possible for you to convert the documentation into forrest? Documentaiton as part of the release and accessible via website docs will really help adoption of hedwig.

        Show
        Mahadev konar added a comment - erwin/ben, Would it be possible for you to convert the documentation into forrest? Documentaiton as part of the release and accessible via website docs will really help adoption of hedwig.
        Hide
        Benjamin Reed added a comment -

        can we do the forrest doc as a separate patch? it's already quite large as it is.

        Show
        Benjamin Reed added a comment - can we do the forrest doc as a separate patch? it's already quite large as it is.
        Hide
        Hadoop QA added a comment -

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

        -1 @author. The patch appears to contain 14 @author tags which the ZooKeeper community has agreed to not allow in code contributions.

        +1 tests included. The patch appears to include 143 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 32 release audit warnings (more than the trunk's current 18 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-h7.grid.sp2.yahoo.net/99/testReport/
        Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/99/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/99/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/99/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/12450515/ZOOKEEPER-775.patch against trunk revision 980576. -1 @author. The patch appears to contain 14 @author tags which the ZooKeeper community has agreed to not allow in code contributions. +1 tests included. The patch appears to include 143 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 32 release audit warnings (more than the trunk's current 18 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-h7.grid.sp2.yahoo.net/99/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/99/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/99/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/99/console This message is automatically generated.
        Hide
        Erwin Tam added a comment -

        Newer patch removing the @author tags in the java classes and adding Apache license headers to the ones that need them. Hudson complained about release audit warnings for some text files but we don't usually add Apache license headers to these type of files. Will see if the autobuild will complain about that with the newer patch.

        Show
        Erwin Tam added a comment - Newer patch removing the @author tags in the java classes and adding Apache license headers to the ones that need them. Hudson complained about release audit warnings for some text files but we don't usually add Apache license headers to these type of files. Will see if the autobuild will complain about that with the newer patch.
        Hide
        Mahadev konar added a comment -

        erwin,
        I think the below might be an oversight but you will have to get rid of this:

        Index: src/contrib/hedwig/NOTICE.txt
        ===================================================================
        --- src/contrib/hedwig/NOTICE.txt	(revision 0)
        +++ src/contrib/hedwig/NOTICE.txt	(revision 0)
        @@ -0,0 +1,2 @@
        +Copyright (c) 2010 Yahoo! Inc.  All rights reserved.
        +
        

        The license should be granted to apache.

        Please take a look at the NOTICE.txt in zookeeper main directory. I am still in the process of reviewing the patch but thought i'd comment on this ....

        Show
        Mahadev konar added a comment - erwin, I think the below might be an oversight but you will have to get rid of this: Index: src/contrib/hedwig/NOTICE.txt =================================================================== --- src/contrib/hedwig/NOTICE.txt (revision 0) +++ src/contrib/hedwig/NOTICE.txt (revision 0) @@ -0,0 +1,2 @@ +Copyright (c) 2010 Yahoo! Inc. All rights reserved. + The license should be granted to apache. Please take a look at the NOTICE.txt in zookeeper main directory. I am still in the process of reviewing the patch but thought i'd comment on this ....
        Hide
        Mahadev konar added a comment -

        also, you might want to get rid of this:

        +% Developer's Guide
        +% Yang Zhang
        

        in src/contrib/hedwig/doc/dev.txt

        Other than that this looks good.

        One question about protobufs, is it apache license? What all are we committing in zookeeper svn thats related to protobufs?

        Show
        Mahadev konar added a comment - also, you might want to get rid of this: +% Developer's Guide +% Yang Zhang in src/contrib/hedwig/doc/dev.txt Other than that this looks good. One question about protobufs, is it apache license? What all are we committing in zookeeper svn thats related to protobufs?
        Hide
        Mahadev konar added a comment -

        sorry I am reviewing and making sure I am uploading my comments, it would have been better for me to write it down in a file and then uploaded at the same time .

        • make sure that you list the files which need to be executable in svn. While committing the svn executable flag is usually lost. So, you will have to provide with a list of files that need to have special svn flags.
        Show
        Mahadev konar added a comment - sorry I am reviewing and making sure I am uploading my comments, it would have been better for me to write it down in a file and then uploaded at the same time . make sure that you list the files which need to be executable in svn. While committing the svn executable flag is usually lost. So, you will have to provide with a list of files that need to have special svn flags.
        Hide
        Mahadev konar added a comment -

        one more comment

        • we are checking in some autogenerated files like ltmain.sh and others. Why do we need tocommit those?
        Show
        Mahadev konar added a comment - one more comment we are checking in some autogenerated files like ltmain.sh and others. Why do we need tocommit those?
        Hide
        Ivan Kelly added a comment -

        I've removed ltmain.sh from the svn repo.

        The patch builds fine on trunk, but I did have some problems running the tests which I resolved by upping the file descriptor limits.
        testOwnershipChange(org.apache.hedwig.server.topics.TestZkTopicManager) seems to be failing consistently.

        C++ client builds. Some work still needs to be done to make testing it easier (requires a manual start of a server control daemon currently).

        These are fairly minor things, and I think it's more important to get hedwig into trunk. So...

        +1

        Show
        Ivan Kelly added a comment - I've removed ltmain.sh from the svn repo. The patch builds fine on trunk, but I did have some problems running the tests which I resolved by upping the file descriptor limits. testOwnershipChange(org.apache.hedwig.server.topics.TestZkTopicManager) seems to be failing consistently. C++ client builds. Some work still needs to be done to make testing it easier (requires a manual start of a server control daemon currently). These are fairly minor things, and I think it's more important to get hedwig into trunk. So... +1
        Hide
        Benjamin Reed added a comment -

        i believe the NOTICE file is consistent with: http://apache.org/legal/src-headers.html#header-existingcopyright

        Show
        Benjamin Reed added a comment - i believe the NOTICE file is consistent with: http://apache.org/legal/src-headers.html#header-existingcopyright
        Hide
        Michi Mutsuzaki added a comment -
          1. scripts/ directory
        • Some scripts under ./scripts don't have the executable bit set.
        • I prefer not to use file extension (analyze instead of analyze.py). We might decide to use some other language later on.
          1. doc/ directory
        • Add instruction to set up local maven repository in build.txt:
          $ mkdir -p $HOME/.m2/repository
          $ export M2_REPO=$HOME/.m2/repository
        • Add instruction to install protobuf in build.txt.

        wget http://protobuf.googlecode.com/files/protobuf-2.3.0.tar.gz
        tar xfvz protobuf-2.3.0.tar.gz
        cd protobuf-2.3.0
        ./configure ; make ; sudo make install
        cd java
        mvn install

          1. client directory
        • I couldn't compile cpp client. we should use a compile time flag to check whether tr1 is available, and if not, fall back to
          boost. This page describes how to do that:

        http://www.codesynthesis.com/~boris/blog/2010/05/24/smart-pointers-in-boost-tr1-cxx-x0/

        If this is too much hassle, maybe we can just use boost instead of tr1 for now.

        • Should the c++ client follow the same naming convention as java for file names (publisherimpl.h vs PublisherImpl.h)?
        Show
        Michi Mutsuzaki added a comment - scripts/ directory Some scripts under ./scripts don't have the executable bit set. I prefer not to use file extension (analyze instead of analyze.py). We might decide to use some other language later on. doc/ directory Add instruction to set up local maven repository in build.txt: $ mkdir -p $HOME/.m2/repository $ export M2_REPO=$HOME/.m2/repository Add instruction to install protobuf in build.txt. wget http://protobuf.googlecode.com/files/protobuf-2.3.0.tar.gz tar xfvz protobuf-2.3.0.tar.gz cd protobuf-2.3.0 ./configure ; make ; sudo make install cd java mvn install client directory I couldn't compile cpp client. we should use a compile time flag to check whether tr1 is available, and if not, fall back to boost. This page describes how to do that: http://www.codesynthesis.com/~boris/blog/2010/05/24/smart-pointers-in-boost-tr1-cxx-x0/ If this is too much hassle, maybe we can just use boost instead of tr1 for now. Should the c++ client follow the same naming convention as java for file names (publisherimpl.h vs PublisherImpl.h)?
        Hide
        Ivan Kelly added a comment -

        1. You can't convey executableness in a patch afaik, but when this is to be submitted the submitter shall do it.

        2. I think just specifying that protobuf is needed should be enough. Many linux distros already package it.

        3. We're currently looking into the tr1 issue. We've hit across this issue with some older distros, but this should all be available from gcc 4.0 onwards, which is 3 years old. The optional use of boost is an option we will be looking into though.

        4. I prefer the all lower case for c++ code, as this is generally the approach most open source projects take. For example, everything in /usr/include/c++ is lowercase.

        Show
        Ivan Kelly added a comment - 1. You can't convey executableness in a patch afaik, but when this is to be submitted the submitter shall do it. 2. I think just specifying that protobuf is needed should be enough. Many linux distros already package it. 3. We're currently looking into the tr1 issue. We've hit across this issue with some older distros, but this should all be available from gcc 4.0 onwards, which is 3 years old. The optional use of boost is an option we will be looking into though. 4. I prefer the all lower case for c++ code, as this is generally the approach most open source projects take. For example, everything in /usr/include/c++ is lowercase.
        Hide
        Mahadev konar added a comment -

        ivan,

        this file looks like garbage -

        server/src/main/resources/p12.pass
        

        is this needed?

        Show
        Mahadev konar added a comment - ivan, this file looks like garbage - server/src/main/resources/p12.pass is this needed?
        Hide
        Mahadev konar added a comment -

        also,

        should I remove this file:

        client/src/main/cpp/ltmain.sh
        

        what about licensing on these files

        client/src/main/cpp/m4
        client/src/main/cpp/m4/ax_doxygen.m4
        client/src/main/cpp/m4/ax_jni_include_dir.m4
        

        is it ok to distribute them via apache?

        Show
        Mahadev konar added a comment - also, should I remove this file: client/src/main/cpp/ltmain.sh what about licensing on these files client/src/main/cpp/m4 client/src/main/cpp/m4/ax_doxygen.m4 client/src/main/cpp/m4/ax_jni_include_dir.m4 is it ok to distribute them via apache?
        Hide
        Ivan Kelly added a comment -

        p12.pass is needed for testing, it's something to do with setting up ssl on the server.

        ltmain.sh has already been removed from internal svn.

        ax_doxygen.m4 has a license:
        #

        1. Copyright (c) 2009 Oren Ben-Kiki <oren@ben-kiki.org>
          #
        2. Copying and distribution of this file, with or without modification, are
        3. permitted in any medium without royalty provided the copyright notice
        4. and this notice are preserved. This file is offered as-is, without any
        5. warranty.

        ax_jni_include_dir.m4 should be removed. Ill probably do a submission tonight for other reasons, so ill remove it at that stage.

        Show
        Ivan Kelly added a comment - p12.pass is needed for testing, it's something to do with setting up ssl on the server. ltmain.sh has already been removed from internal svn. ax_doxygen.m4 has a license: # Copyright (c) 2009 Oren Ben-Kiki <oren@ben-kiki.org> # Copying and distribution of this file, with or without modification, are permitted in any medium without royalty provided the copyright notice and this notice are preserved. This file is offered as-is, without any warranty. ax_jni_include_dir.m4 should be removed. Ill probably do a submission tonight for other reasons, so ill remove it at that stage.
        Hide
        Mahadev konar added a comment -

        this patch make the following changes to the sumitted patch.

        • removes ltmain.sh
        • removes client/src/main/cpp/m4/ax_jni_include_dir.m4
        • remove author names
        Show
        Mahadev konar added a comment - this patch make the following changes to the sumitted patch. removes ltmain.sh removes client/src/main/cpp/m4/ax_jni_include_dir.m4 remove author names
        Hide
        Mahadev konar added a comment -

        I just committed this.

        Thanks Ivan, Erwin, Ben. It would be great if you guys can focus on improved documentation for this since it will be critical for adoption of the project.

        Show
        Mahadev konar added a comment - I just committed this. Thanks Ivan, Erwin, Ben. It would be great if you guys can focus on improved documentation for this since it will be critical for adoption of the project.
        Hide
        Patrick Hunt added a comment -

        Awesome job everyone, really psyched to see this get in!

        Show
        Patrick Hunt added a comment - Awesome job everyone, really psyched to see this get in!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #936 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/936/)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #936 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/936/ )
        Hide
        Sudipto Das added a comment -

        I am traveling and will return on Sept 21st. I will be away from my
        email for most of the period. I will get back whenever I get a chance.


        Best Regards
        Sudipto


        Sudipto Das
        PhD Candidate
        CS @ UCSB
        Santa Barbara, CA 93106, USA
        http://www.cs.ucsb.edu/~sudipto

        Show
        Sudipto Das added a comment - I am traveling and will return on Sept 21st. I will be away from my email for most of the period. I will get back whenever I get a chance. – Best Regards Sudipto – Sudipto Das PhD Candidate CS @ UCSB Santa Barbara, CA 93106, USA http://www.cs.ucsb.edu/~sudipto

          People

          • Assignee:
            Benjamin Reed
            Reporter:
            Benjamin Reed
          • Votes:
            0 Vote for this issue
            Watchers:
            16 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development