Menu
▾
▴
zookeeper-developer — Mailing list to discuss development of zookeeper
You can subscribe to this list here.
| 2007 |
Jan
|
Feb
|
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
|
Dec
(37) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 2008 |
Jan
(27) |
Feb
(34) |
Mar
(30) |
Apr
(151) |
May
(184) |
Jun
(55) |
Jul
(2) |
Aug
(6) |
Sep
|
Oct
(1) |
Nov
|
Dec
|
|
From: SourceForge.net <no...@so...> - 2008-05-09 19:53:51
|
Patches item #1956499, was opened at 2008-05-02 16:58 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None >Status: Closed Resolution: Accepted Priority: 7 Private: No Submitted By: Andrew Kornev (akornev) Assigned to: Benjamin Reed (breed) Summary: the dist build target Initial Comment: The patch adds a new build target to the ant build file -- "dist". The target builds all of the zookeeper distribution tar files in one shot: java source, java binary and c client source. Once the build has finished, the tar files are located in ${basedir}/distribution and ready to be uploaded to the distribution server. ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-09 12:46 Message: Logged In: YES user_id=1926652 Originator: YES Committed at revision 159 ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-09 11:39 Message: Logged In: YES user_id=1926680 Originator: NO +1 looks good... ben is it ok with you ? ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-08 18:47 Message: Logged In: YES user_id=1926652 Originator: YES This patch fixes a bug that causes the entire source tree to be included in the server jar file. File Added: build.xml.patch ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 12:20 Message: Logged In: YES user_id=1926652 Originator: YES Committed at revision 153 ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 16:56 Message: Logged In: YES user_id=1926652 Originator: YES This patch is the same as the previous one except, in anticipation of the [1951806] patch going in, the java-src and java-bin packages include the bin/* directory. File Added: build-3.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-06 16:37 Message: Logged In: YES user_id=154690 Originator: NO +1 Looks good. It worked for me. I think we need to rev it before committing. [ 1951806 ] Sample startup script Needs to go in first and the bin directory should be included in the java-bin tar.gz ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 16:06 Message: Logged In: YES user_id=1926652 Originator: YES 1) the target JDK version is a property now. By default, it's set to the version of the compiler. It can be overwritten from the command line: "-Dtarget.jdk=1.x". The new JMX feature is only available for JDK 1.6. 2) fixes bug [1958183] where the build fails because of an invalid SVN revision number. The "dist" target generates "revision.properties" file that is then included in the java source tar. The presence of the properties file supresses the SVN revision check during subsequent builds from the source tar. 3) fixed an incomplete error message issued by the "dist" target build if the C client Makefile is not present. 4) the "dist" target now produces a unified zookeeper-x.y.x.tar.gz file in the project basedir as well as separate java and C package files under the distribution/ directory. 5) updated the current release number to 2.2.0 File Added: build-2.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 12:08 Message: Logged In: YES user_id=12853 Originator: NO I'm unable to build dist on my linux machine: BUILD FAILED build.xml:227: The following error occurred while executing this line: build.xml:200: Please run 'autoreconf -i' in c/ irectory to generate a Makefile. I've run "autoreconf -i" a number of times here is the output: :~/workspaces/zookeeper/c$ autoreconf -i libtoolize: `config.guess' exists: use `--force' to overwrite libtoolize: `config.sub' exists: use `--force' to overwrite libtoolize: `ltmain.sh' exists: use `--force' to overwrite Makefile.am:60: wildcard tests/*.cc: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:60: wildcard tests/*.h: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:67: shell cat tests/wrappers.opt: non-POSIX variable name Makefile.am:67: (probably a GNU make extension) Makefile.am:80: shell cat tests/wrappers-mt.opt: non-POSIX variable name Makefile.am:80: (probably a GNU make extension) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 11:54 Message: Logged In: YES user_id=12853 Originator: NO Please remove requirement for java 1.6 (back to 1.5) for the time being. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:42 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-03 16:12 Message: Logged In: YES user_id=154690 Originator: NO Looks good Andrew. I don't think we should change to Java 1.6 though until we have to. The poor Mac users have to suffer with an Java 1.5. (Although the Mac koolaid seems to allow them to forget their suffering :) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-09 18:39:33
|
Patches item #1956499, was opened at 2008-05-02 23:58 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: Accepted Priority: 7 Private: No Submitted By: Andrew Kornev (akornev) Assigned to: Benjamin Reed (breed) Summary: the dist build target Initial Comment: The patch adds a new build target to the ant build file -- "dist". The target builds all of the zookeeper distribution tar files in one shot: java source, java binary and c client source. Once the build has finished, the tar files are located in ${basedir}/distribution and ready to be uploaded to the distribution server. ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-09 18:39 Message: Logged In: YES user_id=1926680 Originator: NO +1 looks good... ben is it ok with you ? ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-09 01:47 Message: Logged In: YES user_id=1926652 Originator: YES This patch fixes a bug that causes the entire source tree to be included in the server jar file. File Added: build.xml.patch ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 19:20 Message: Logged In: YES user_id=1926652 Originator: YES Committed at revision 153 ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 23:56 Message: Logged In: YES user_id=1926652 Originator: YES This patch is the same as the previous one except, in anticipation of the [1951806] patch going in, the java-src and java-bin packages include the bin/* directory. File Added: build-3.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-06 23:37 Message: Logged In: YES user_id=154690 Originator: NO +1 Looks good. It worked for me. I think we need to rev it before committing. [ 1951806 ] Sample startup script Needs to go in first and the bin directory should be included in the java-bin tar.gz ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 23:06 Message: Logged In: YES user_id=1926652 Originator: YES 1) the target JDK version is a property now. By default, it's set to the version of the compiler. It can be overwritten from the command line: "-Dtarget.jdk=1.x". The new JMX feature is only available for JDK 1.6. 2) fixes bug [1958183] where the build fails because of an invalid SVN revision number. The "dist" target generates "revision.properties" file that is then included in the java source tar. The presence of the properties file supresses the SVN revision check during subsequent builds from the source tar. 3) fixed an incomplete error message issued by the "dist" target build if the C client Makefile is not present. 4) the "dist" target now produces a unified zookeeper-x.y.x.tar.gz file in the project basedir as well as separate java and C package files under the distribution/ directory. 5) updated the current release number to 2.2.0 File Added: build-2.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:08 Message: Logged In: YES user_id=12853 Originator: NO I'm unable to build dist on my linux machine: BUILD FAILED build.xml:227: The following error occurred while executing this line: build.xml:200: Please run 'autoreconf -i' in c/ irectory to generate a Makefile. I've run "autoreconf -i" a number of times here is the output: :~/workspaces/zookeeper/c$ autoreconf -i libtoolize: `config.guess' exists: use `--force' to overwrite libtoolize: `config.sub' exists: use `--force' to overwrite libtoolize: `ltmain.sh' exists: use `--force' to overwrite Makefile.am:60: wildcard tests/*.cc: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:60: wildcard tests/*.h: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:67: shell cat tests/wrappers.opt: non-POSIX variable name Makefile.am:67: (probably a GNU make extension) Makefile.am:80: shell cat tests/wrappers-mt.opt: non-POSIX variable name Makefile.am:80: (probably a GNU make extension) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:54 Message: Logged In: YES user_id=12853 Originator: NO Please remove requirement for java 1.6 (back to 1.5) for the time being. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:42 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-03 23:12 Message: Logged In: YES user_id=154690 Originator: NO Looks good Andrew. I don't think we should change to Java 1.6 though until we have to. The poor Mac users have to suffer with an Java 1.5. (Although the Mac koolaid seems to allow them to forget their suffering :) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-09 18:34:21
|
Patches item #1913998, was opened at 2008-03-13 18:52 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1913998&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Andrew Kornev (akornev) Assigned to: Nobody/Anonymous (nobody) Summary: JMX instrumentation Initial Comment: This patch adds support for management and monitoring via JMX. It uses the MXBean feature made available in Java 6. The instrumentation code relies on the new lifecycle event notification mechanism to dynamically register and unregister MBeans. The ant build file has NOT been modified to build this new feature yet. There are no javadocs for the new classes either (but this will be fixed, of course). ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-09 11:34 Message: Logged In: YES user_id=1926652 Originator: YES Fixed compilation errors introduced as a result of [1956480] patch. Now is really a good time to start reviewing this patch. File Added: jmx-4.patch ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 11:25 Message: Logged In: YES user_id=1926652 Originator: YES Moved around ManagedZooKeeperServer and ManagedQuorumPeer classes back to the jmx source tree to make it possible to build the src source tree whithout the jmx tree. Now would be a good time to start reviewing this patch. File Added: jmx-3.patch ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-01 18:06 Message: Logged In: YES user_id=1926652 Originator: YES Added javadocs, updated the code to use log4j, moved common classes to the src/ directory tree. File Added: jmx-2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-17 13:44 Message: Logged In: YES user_id=1926680 Originator: NO that is primarily my concern. Meaning if jmx is to be enabled in production then creating an ojbect per connection would deteriorate the scalability of zookeeper. What I mean is that do we actually need per connection management in jmx? or we could have just connection stats in jmx and some other means to debug problems with connections (other than jmx)? ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-03-17 13:14 Message: Logged In: YES user_id=1926652 Originator: YES JMX is primarily for management and monitoring, and my hope is to have JMX enabled in production by default. JMX instrumentation code will eventually rely on the /proc data tree (when it becomes available) to get connection-related information. But we're still gonna have to have a bean per connection registered with MBeanServer. Dynamic registering/unregistering of connection beans may indeed incur some (insignificant) overhead, but I'm not aware of any other way of making a dynamic resource avilable to JMX. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-17 12:50 Message: Logged In: YES user_id=1926680 Originator: NO 3) for this what I meant was that .. we c annot run with jmx on in production .. and if connection tracking is for debugging purpose then jmx just does not sound the right tool for debugging... 4) thats true... but lets have a better process in line :) ... sorry to have started with you. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-03-17 12:23 Message: Logged In: YES user_id=1926652 Originator: YES 1) yes! 2) updated the code to use logError() for logging errors only and logTraceMessage() for logging debug/info. 3) not sure I understand the concern. Connection tracking using /proc FS is orthogonal to JMX. JMX Beans would have to be instantiated for each new connection anyway if we want to be able to manage connections thru JMX. 4) this may take a while. Historically, however, lacking and/or misleading javadocs have never been a showstopper for the existing ZK code ;-) ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-15 11:02 Message: Logged In: YES user_id=1926680 Originator: NO 1) dataTreeBean = new com.yahoo.zookeeper.jmx.server.DataTreeBean( + server.dataTree); can we just replace this with DataTreeBean(datatree)? 2) also i wonder if we should have logging levels on the server side like debug and info , warn, fatal right now we just have logerror and logwarn... and we end up using logError for everything .. 3) am really unconfortable with the idea that we keep track of all the connections using jmx beans? I thought we were going to put all this info in the /proc file system? We end up creating a new object connection bean for every connection (as per my understanding) ... 4) can you upload a patch with javadocs? :) Its easier to review :). ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-03-14 10:11 Message: Logged In: YES user_id=1926652 Originator: YES Are you sure it's this patch that has tabs in it? Or, maybe, you meant the other one (the refactoring patch)? Because that one does have tabs (which I've already fixed in locally). But this one does not. Thanks for reviewing! ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-13 19:20 Message: Logged In: YES user_id=1926680 Originator: NO this patch has tabs in it... i havent finished reviewing... just getting my review comments out... ill let you know when I am done fully reviewing the patch so that you dont have to upload it again and again. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1913998&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-09 01:47:13
|
Patches item #1956499, was opened at 2008-05-02 16:58 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None >Status: Open Resolution: Accepted Priority: 7 Private: No Submitted By: Andrew Kornev (akornev) Assigned to: Benjamin Reed (breed) Summary: the dist build target Initial Comment: The patch adds a new build target to the ant build file -- "dist". The target builds all of the zookeeper distribution tar files in one shot: java source, java binary and c client source. Once the build has finished, the tar files are located in ${basedir}/distribution and ready to be uploaded to the distribution server. ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-08 18:47 Message: Logged In: YES user_id=1926652 Originator: YES This patch fixes a bug that causes the entire source tree to be included in the server jar file. File Added: build.xml.patch ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 12:20 Message: Logged In: YES user_id=1926652 Originator: YES Committed at revision 153 ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 16:56 Message: Logged In: YES user_id=1926652 Originator: YES This patch is the same as the previous one except, in anticipation of the [1951806] patch going in, the java-src and java-bin packages include the bin/* directory. File Added: build-3.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-06 16:37 Message: Logged In: YES user_id=154690 Originator: NO +1 Looks good. It worked for me. I think we need to rev it before committing. [ 1951806 ] Sample startup script Needs to go in first and the bin directory should be included in the java-bin tar.gz ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 16:06 Message: Logged In: YES user_id=1926652 Originator: YES 1) the target JDK version is a property now. By default, it's set to the version of the compiler. It can be overwritten from the command line: "-Dtarget.jdk=1.x". The new JMX feature is only available for JDK 1.6. 2) fixes bug [1958183] where the build fails because of an invalid SVN revision number. The "dist" target generates "revision.properties" file that is then included in the java source tar. The presence of the properties file supresses the SVN revision check during subsequent builds from the source tar. 3) fixed an incomplete error message issued by the "dist" target build if the C client Makefile is not present. 4) the "dist" target now produces a unified zookeeper-x.y.x.tar.gz file in the project basedir as well as separate java and C package files under the distribution/ directory. 5) updated the current release number to 2.2.0 File Added: build-2.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 12:08 Message: Logged In: YES user_id=12853 Originator: NO I'm unable to build dist on my linux machine: BUILD FAILED build.xml:227: The following error occurred while executing this line: build.xml:200: Please run 'autoreconf -i' in c/ irectory to generate a Makefile. I've run "autoreconf -i" a number of times here is the output: :~/workspaces/zookeeper/c$ autoreconf -i libtoolize: `config.guess' exists: use `--force' to overwrite libtoolize: `config.sub' exists: use `--force' to overwrite libtoolize: `ltmain.sh' exists: use `--force' to overwrite Makefile.am:60: wildcard tests/*.cc: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:60: wildcard tests/*.h: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:67: shell cat tests/wrappers.opt: non-POSIX variable name Makefile.am:67: (probably a GNU make extension) Makefile.am:80: shell cat tests/wrappers-mt.opt: non-POSIX variable name Makefile.am:80: (probably a GNU make extension) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 11:54 Message: Logged In: YES user_id=12853 Originator: NO Please remove requirement for java 1.6 (back to 1.5) for the time being. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:42 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-03 16:12 Message: Logged In: YES user_id=154690 Originator: NO Looks good Andrew. I don't think we should change to Java 1.6 though until we have to. The poor Mac users have to suffer with an Java 1.5. (Although the Mac koolaid seems to allow them to forget their suffering :) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 21:51:47
|
Patches item #1956480, was opened at 2008-05-02 15:48 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None >Status: Closed >Resolution: Accepted Priority: 7 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Andrew Kornev (akornev) Summary: cleanup trace logging Initial Comment: Cleanup of trace logging. Renamed ZooLog to ZooTrace. Major cleanup of tracing. When this is merged please review the trace messages - it looks like a number of them are really debug/info level messages - they should be updated appropriately. This should be the last major change for log4j integration - other than adding more info/debug/trace messages as appropriate. ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-08 14:51 Message: Logged In: YES user_id=1926652 Originator: NO +1 Committed at revision 156 ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-08 14:34 Message: Logged In: YES user_id=12853 Originator: YES File Added: tracecleanup3.patch.gz ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-07 15:09 Message: Logged In: YES user_id=12853 Originator: YES The first issue you mention is a common pattern for logging code - the "istraceenabled" eliminates the string concatenation expense when tracing is not turned on (optimization). (should not be changed) ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 14:50 Message: Logged In: YES user_id=1926652 Originator: NO Other than a few things mentioned below, the patch looks good. ClientCnxn.java: This check to see if trace is enabled seems to be redundant as logTraceMessage() performs the same check: long traceMask = ZooTrace.SESSION_TRACE_MASK; if (ZooTrace.isTraceEnabled(LOG, traceMask)) { ZooTrace.logTraceMessage(LOG, traceMask, "Close ClientCnxn for session: " + sessionId + "!"); } util/Profiler.java: Unused TraceLog import, LOG severity should be warn not error Unused TraceLog import: AuthFastLeaderElection.java FastLeaderElection.java FollowerZooKeeperServer.java Leader.java LeaderElection.java QuorumCnxManager.java QuorumPeerConfig.java SendAckRequestProcessor.java ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:43 Message: Logged In: YES user_id=12853 Originator: YES The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 21:34:24
|
Patches item #1956480, was opened at 2008-05-02 15:48 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Andrew Kornev (akornev) Summary: cleanup trace logging Initial Comment: Cleanup of trace logging. Renamed ZooLog to ZooTrace. Major cleanup of tracing. When this is merged please review the trace messages - it looks like a number of them are really debug/info level messages - they should be updated appropriately. This should be the last major change for log4j integration - other than adding more info/debug/trace messages as appropriate. ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-08 14:34 Message: Logged In: YES user_id=12853 Originator: YES File Added: tracecleanup3.patch.gz ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-07 15:09 Message: Logged In: YES user_id=12853 Originator: YES The first issue you mention is a common pattern for logging code - the "istraceenabled" eliminates the string concatenation expense when tracing is not turned on (optimization). (should not be changed) ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 14:50 Message: Logged In: YES user_id=1926652 Originator: NO Other than a few things mentioned below, the patch looks good. ClientCnxn.java: This check to see if trace is enabled seems to be redundant as logTraceMessage() performs the same check: long traceMask = ZooTrace.SESSION_TRACE_MASK; if (ZooTrace.isTraceEnabled(LOG, traceMask)) { ZooTrace.logTraceMessage(LOG, traceMask, "Close ClientCnxn for session: " + sessionId + "!"); } util/Profiler.java: Unused TraceLog import, LOG severity should be warn not error Unused TraceLog import: AuthFastLeaderElection.java FastLeaderElection.java FollowerZooKeeperServer.java Leader.java LeaderElection.java QuorumCnxManager.java QuorumPeerConfig.java SendAckRequestProcessor.java ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:43 Message: Logged In: YES user_id=12853 Originator: YES The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 21:18:50
|
Patches item #1956801, was opened at 2008-05-03 17:00 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Closed Resolution: Fixed Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Small fixes to Authenticated LE Initial Comment: * Removed unused variable; * Removed a couple of naked notifies; * Fixed a type cast; * Removed an unnecessary exception catch; * Replaced numerical constructor with valueOf in a couple of places. ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 21:18 Message: Logged In: YES user_id=1926680 Originator: NO Committed revision 155. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 20:56 Message: Logged In: YES user_id=1926680 Originator: NO Committed revision 155. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-08 20:25 Message: Logged In: YES user_id=12853 Originator: NO +1 - fine w/me (this comment is on patch v2 from the downloads, not patchv1) ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 20:06 Message: Logged In: YES user_id=1926680 Originator: NO looks good.. pat you ok with it? ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 01:16 Message: Logged In: YES user_id=1926444 Originator: YES Have taken the following actions: * Put back the synchronized blocks; * Have double-checked that the naked notifies are actually ok. Findbugs complains about no change to the state of the object, which is true. The notify is triggered by the reception of a message, though, which is an external event, and that's why there is no state change upon the execution of the notify. File Added: patch-auth-le ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:50 Message: Logged In: YES user_id=12853 Originator: NO 1) On line 214/339 synchronization was removed, don't you have to hold the monitor when calling notify? "This method should only be called by a thread that is the owner of this object's monitor." 2) Remove unused field line 377 3) This is a new one to me -- Findbugs has an interesting comment for lines 536, 567 "[DL] Synchronization on boxed primative could lead to deadlock [DL_SYNCHRONIZATION_ON_BOXED_PRIMITIVE] The code synchronizes on a boxed primitive constant, such as an Integer. private static Integer count = 0; ... synchronized(count) { count++; } ... Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock" ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:41 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 21:06:16
|
Patches item #1958274, was opened at 2008-05-05 21:16 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Closed Resolution: Fixed Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 21:06 Message: Logged In: YES user_id=1926680 Originator: NO yes . thats how I applied the patch. Its generally a good practice to upload a patch from the trunk so that the reviewer and the committer can just do patch -p0 and be done with it.. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-08 20:22 Message: Logged In: YES user_id=12853 Originator: NO Mahadev re point 1 - just use the "patch -p# < patchfile" option, it will correctly strip the prefix (you need to tell it what depth via the #, -p3 strips 3 from the prefix dirhier). ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 18:53 Message: Logged In: YES user_id=1926680 Originator: NO ok ... i updated the patch myself ... committed to revision 154 but two points for future flavio :)-- 1) the diff should be generated from the zookeeper directory. this patch had diff from C:/ something 2) make sure that the patch does not have any windows characters. also another bug would be to open some indentation issues in the original code in fast le (not in the patch). ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 18:42 Message: Logged In: YES user_id=1926680 Originator: NO +1 looks good but the only problem is with ^M charaters present in the patch. Can you get rid of this? it usually happens on windows files that they have ^M charaters at the end of line... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-07 05:44 Message: Logged In: YES user_id=12853 Originator: NO Flavio please delete the old patch(es). ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 05:17 Message: Logged In: YES user_id=1926444 Originator: YES File Added: patch-fle.diff ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 05:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 02:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 20:56:47
|
Patches item #1956801, was opened at 2008-05-03 17:00 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None >Status: Closed >Resolution: Fixed Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Small fixes to Authenticated LE Initial Comment: * Removed unused variable; * Removed a couple of naked notifies; * Fixed a type cast; * Removed an unnecessary exception catch; * Replaced numerical constructor with valueOf in a couple of places. ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 20:56 Message: Logged In: YES user_id=1926680 Originator: NO Committed revision 155. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-08 20:25 Message: Logged In: YES user_id=12853 Originator: NO +1 - fine w/me (this comment is on patch v2 from the downloads, not patchv1) ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 20:06 Message: Logged In: YES user_id=1926680 Originator: NO looks good.. pat you ok with it? ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 01:16 Message: Logged In: YES user_id=1926444 Originator: YES Have taken the following actions: * Put back the synchronized blocks; * Have double-checked that the naked notifies are actually ok. Findbugs complains about no change to the state of the object, which is true. The notify is triggered by the reception of a message, though, which is an external event, and that's why there is no state change upon the execution of the notify. File Added: patch-auth-le ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:50 Message: Logged In: YES user_id=12853 Originator: NO 1) On line 214/339 synchronization was removed, don't you have to hold the monitor when calling notify? "This method should only be called by a thread that is the owner of this object's monitor." 2) Remove unused field line 377 3) This is a new one to me -- Findbugs has an interesting comment for lines 536, 567 "[DL] Synchronization on boxed primative could lead to deadlock [DL_SYNCHRONIZATION_ON_BOXED_PRIMITIVE] The code synchronizes on a boxed primitive constant, such as an Integer. private static Integer count = 0; ... synchronized(count) { count++; } ... Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock" ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:41 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 20:25:11
|
Patches item #1956801, was opened at 2008-05-03 10:00 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Small fixes to Authenticated LE Initial Comment: * Removed unused variable; * Removed a couple of naked notifies; * Fixed a type cast; * Removed an unnecessary exception catch; * Replaced numerical constructor with valueOf in a couple of places. ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-08 13:25 Message: Logged In: YES user_id=12853 Originator: NO +1 - fine w/me (this comment is on patch v2 from the downloads, not patchv1) ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 13:06 Message: Logged In: YES user_id=1926680 Originator: NO looks good.. pat you ok with it? ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-06 18:16 Message: Logged In: YES user_id=1926444 Originator: YES Have taken the following actions: * Put back the synchronized blocks; * Have double-checked that the naked notifies are actually ok. Findbugs complains about no change to the state of the object, which is true. The notify is triggered by the reception of a message, though, which is an external event, and that's why there is no state change upon the execution of the notify. File Added: patch-auth-le ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 11:50 Message: Logged In: YES user_id=12853 Originator: NO 1) On line 214/339 synchronization was removed, don't you have to hold the monitor when calling notify? "This method should only be called by a thread that is the owner of this object's monitor." 2) Remove unused field line 377 3) This is a new one to me -- Findbugs has an interesting comment for lines 536, 567 "[DL] Synchronization on boxed primative could lead to deadlock [DL_SYNCHRONIZATION_ON_BOXED_PRIMITIVE] The code synchronizes on a boxed primitive constant, such as an Integer. private static Integer count = 0; ... synchronized(count) { count++; } ... Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock" ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:41 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 20:22:44
|
Patches item #1958274, was opened at 2008-05-05 14:16 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Closed Resolution: Fixed Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-08 13:22 Message: Logged In: YES user_id=12853 Originator: NO Mahadev re point 1 - just use the "patch -p# < patchfile" option, it will correctly strip the prefix (you need to tell it what depth via the #, -p3 strips 3 from the prefix dirhier). ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 11:53 Message: Logged In: YES user_id=1926680 Originator: NO ok ... i updated the patch myself ... committed to revision 154 but two points for future flavio :)-- 1) the diff should be generated from the zookeeper directory. this patch had diff from C:/ something 2) make sure that the patch does not have any windows characters. also another bug would be to open some indentation issues in the original code in fast le (not in the patch). ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 11:42 Message: Logged In: YES user_id=1926680 Originator: NO +1 looks good but the only problem is with ^M charaters present in the patch. Can you get rid of this? it usually happens on windows files that they have ^M charaters at the end of line... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 22:44 Message: Logged In: YES user_id=12853 Originator: NO Flavio please delete the old patch(es). ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-06 22:17 Message: Logged In: YES user_id=1926444 Originator: YES File Added: patch-fle.diff ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-06 22:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 19:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 11:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 20:06:30
|
Patches item #1956801, was opened at 2008-05-03 17:00 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Small fixes to Authenticated LE Initial Comment: * Removed unused variable; * Removed a couple of naked notifies; * Fixed a type cast; * Removed an unnecessary exception catch; * Replaced numerical constructor with valueOf in a couple of places. ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 20:06 Message: Logged In: YES user_id=1926680 Originator: NO looks good.. pat you ok with it? ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 01:16 Message: Logged In: YES user_id=1926444 Originator: YES Have taken the following actions: * Put back the synchronized blocks; * Have double-checked that the naked notifies are actually ok. Findbugs complains about no change to the state of the object, which is true. The notify is triggered by the reception of a message, though, which is an external event, and that's why there is no state change upon the execution of the notify. File Added: patch-auth-le ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:50 Message: Logged In: YES user_id=12853 Originator: NO 1) On line 214/339 synchronization was removed, don't you have to hold the monitor when calling notify? "This method should only be called by a thread that is the owner of this object's monitor." 2) Remove unused field line 377 3) This is a new one to me -- Findbugs has an interesting comment for lines 536, 567 "[DL] Synchronization on boxed primative could lead to deadlock [DL_SYNCHRONIZATION_ON_BOXED_PRIMITIVE] The code synchronizes on a boxed primitive constant, such as an Integer. private static Integer count = 0; ... synchronized(count) { count++; } ... Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock" ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:41 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 18:53:22
|
Patches item #1958274, was opened at 2008-05-05 21:16 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None >Status: Closed >Resolution: Fixed Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 18:53 Message: Logged In: YES user_id=1926680 Originator: NO ok ... i updated the patch myself ... committed to revision 154 but two points for future flavio :)-- 1) the diff should be generated from the zookeeper directory. this patch had diff from C:/ something 2) make sure that the patch does not have any windows characters. also another bug would be to open some indentation issues in the original code in fast le (not in the patch). ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 18:42 Message: Logged In: YES user_id=1926680 Originator: NO +1 looks good but the only problem is with ^M charaters present in the patch. Can you get rid of this? it usually happens on windows files that they have ^M charaters at the end of line... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-07 05:44 Message: Logged In: YES user_id=12853 Originator: NO Flavio please delete the old patch(es). ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 05:17 Message: Logged In: YES user_id=1926444 Originator: YES File Added: patch-fle.diff ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 05:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 02:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-08 18:41:58
|
Patches item #1958274, was opened at 2008-05-05 21:16 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-08 18:42 Message: Logged In: YES user_id=1926680 Originator: NO +1 looks good but the only problem is with ^M charaters present in the patch. Can you get rid of this? it usually happens on windows files that they have ^M charaters at the end of line... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-07 05:44 Message: Logged In: YES user_id=12853 Originator: NO Flavio please delete the old patch(es). ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 05:17 Message: Logged In: YES user_id=1926444 Originator: YES File Added: patch-fle.diff ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 05:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 02:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 22:08:56
|
Patches item #1956480, was opened at 2008-05-02 15:48 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Andrew Kornev (akornev) Summary: cleanup trace logging Initial Comment: Cleanup of trace logging. Renamed ZooLog to ZooTrace. Major cleanup of tracing. When this is merged please review the trace messages - it looks like a number of them are really debug/info level messages - they should be updated appropriately. This should be the last major change for log4j integration - other than adding more info/debug/trace messages as appropriate. ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-07 15:09 Message: Logged In: YES user_id=12853 Originator: YES The first issue you mention is a common pattern for logging code - the "istraceenabled" eliminates the string concatenation expense when tracing is not turned on (optimization). (should not be changed) ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-07 14:50 Message: Logged In: YES user_id=1926652 Originator: NO Other than a few things mentioned below, the patch looks good. ClientCnxn.java: This check to see if trace is enabled seems to be redundant as logTraceMessage() performs the same check: long traceMask = ZooTrace.SESSION_TRACE_MASK; if (ZooTrace.isTraceEnabled(LOG, traceMask)) { ZooTrace.logTraceMessage(LOG, traceMask, "Close ClientCnxn for session: " + sessionId + "!"); } util/Profiler.java: Unused TraceLog import, LOG severity should be warn not error Unused TraceLog import: AuthFastLeaderElection.java FastLeaderElection.java FollowerZooKeeperServer.java Leader.java LeaderElection.java QuorumCnxManager.java QuorumPeerConfig.java SendAckRequestProcessor.java ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:43 Message: Logged In: YES user_id=12853 Originator: YES The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 21:49:54
|
Patches item #1956480, was opened at 2008-05-02 15:48 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Andrew Kornev (akornev) Summary: cleanup trace logging Initial Comment: Cleanup of trace logging. Renamed ZooLog to ZooTrace. Major cleanup of tracing. When this is merged please review the trace messages - it looks like a number of them are really debug/info level messages - they should be updated appropriately. This should be the last major change for log4j integration - other than adding more info/debug/trace messages as appropriate. ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-07 14:50 Message: Logged In: YES user_id=1926652 Originator: NO Other than a few things mentioned below, the patch looks good. ClientCnxn.java: This check to see if trace is enabled seems to be redundant as logTraceMessage() performs the same check: long traceMask = ZooTrace.SESSION_TRACE_MASK; if (ZooTrace.isTraceEnabled(LOG, traceMask)) { ZooTrace.logTraceMessage(LOG, traceMask, "Close ClientCnxn for session: " + sessionId + "!"); } util/Profiler.java: Unused TraceLog import, LOG severity should be warn not error Unused TraceLog import: AuthFastLeaderElection.java FastLeaderElection.java FollowerZooKeeperServer.java Leader.java LeaderElection.java QuorumCnxManager.java QuorumPeerConfig.java SendAckRequestProcessor.java ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:43 Message: Logged In: YES user_id=12853 Originator: YES The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956480&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 19:20:59
|
Patches item #1956499, was opened at 2008-05-02 16:58 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None >Status: Closed >Resolution: Accepted Priority: 7 Private: No Submitted By: Andrew Kornev (akornev) Assigned to: Benjamin Reed (breed) Summary: the dist build target Initial Comment: The patch adds a new build target to the ant build file -- "dist". The target builds all of the zookeeper distribution tar files in one shot: java source, java binary and c client source. Once the build has finished, the tar files are located in ${basedir}/distribution and ready to be uploaded to the distribution server. ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-07 12:20 Message: Logged In: YES user_id=1926652 Originator: YES Committed at revision 153 ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 16:56 Message: Logged In: YES user_id=1926652 Originator: YES This patch is the same as the previous one except, in anticipation of the [1951806] patch going in, the java-src and java-bin packages include the bin/* directory. File Added: build-3.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-06 16:37 Message: Logged In: YES user_id=154690 Originator: NO +1 Looks good. It worked for me. I think we need to rev it before committing. [ 1951806 ] Sample startup script Needs to go in first and the bin directory should be included in the java-bin tar.gz ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-06 16:06 Message: Logged In: YES user_id=1926652 Originator: YES 1) the target JDK version is a property now. By default, it's set to the version of the compiler. It can be overwritten from the command line: "-Dtarget.jdk=1.x". The new JMX feature is only available for JDK 1.6. 2) fixes bug [1958183] where the build fails because of an invalid SVN revision number. The "dist" target generates "revision.properties" file that is then included in the java source tar. The presence of the properties file supresses the SVN revision check during subsequent builds from the source tar. 3) fixed an incomplete error message issued by the "dist" target build if the C client Makefile is not present. 4) the "dist" target now produces a unified zookeeper-x.y.x.tar.gz file in the project basedir as well as separate java and C package files under the distribution/ directory. 5) updated the current release number to 2.2.0 File Added: build-2.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 12:08 Message: Logged In: YES user_id=12853 Originator: NO I'm unable to build dist on my linux machine: BUILD FAILED build.xml:227: The following error occurred while executing this line: build.xml:200: Please run 'autoreconf -i' in c/ irectory to generate a Makefile. I've run "autoreconf -i" a number of times here is the output: :~/workspaces/zookeeper/c$ autoreconf -i libtoolize: `config.guess' exists: use `--force' to overwrite libtoolize: `config.sub' exists: use `--force' to overwrite libtoolize: `ltmain.sh' exists: use `--force' to overwrite Makefile.am:60: wildcard tests/*.cc: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:60: wildcard tests/*.h: non-POSIX variable name Makefile.am:60: (probably a GNU make extension) Makefile.am:67: shell cat tests/wrappers.opt: non-POSIX variable name Makefile.am:67: (probably a GNU make extension) Makefile.am:80: shell cat tests/wrappers-mt.opt: non-POSIX variable name Makefile.am:80: (probably a GNU make extension) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 11:54 Message: Logged In: YES user_id=12853 Originator: NO Please remove requirement for java 1.6 (back to 1.5) for the time being. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:42 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-03 16:12 Message: Logged In: YES user_id=154690 Originator: NO Looks good Andrew. I don't think we should change to Java 1.6 though until we have to. The poor Mac users have to suffer with an Java 1.5. (Although the Mac koolaid seems to allow them to forget their suffering :) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956499&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 18:45:51
|
Patches item #1951806, was opened at 2008-04-25 17:10 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1951806&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Mahadev Konar (mahadevkonar) Summary: Sample startup script Initial Comment: This may be useful as a temporary script. Eventually there are several things that should be improved: a) the log rotation hack should go away when real logging get implemented b) the kill hack should go away when there is a good way to kill the process. In the meantime, this may be handy to people at least as a starting point. ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 18:45 Message: Logged In: YES user_id=1926680 Originator: NO Committed revision 152. committed the patch. Thanks Ben and Ted!! Am not closing the bug -- Ill update the twiki and then close the bug. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-07 18:39 Message: Logged In: YES user_id=154690 Originator: NO +1 make it happen ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 22:58 Message: Logged In: YES user_id=1926680 Originator: NO i am uploading a patch that makes log4j configurable in the scripts and also made changes to the script so that it works in the developer environment as well . File Added: log4j_build_script_1.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:46 Message: Logged In: YES user_id=12853 Originator: NO +1 but should also include update to the wiki to document: a) the commands b) usage, in particular env variables and default expected locations (for example ZOOLIBDIR is bin/../lib + should have all necessary libs, zoo.cfg + log4j.properties are in ZOOCFGDIR, etc...) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:43 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 06:22 Message: Logged In: YES user_id=1926680 Originator: NO nice scripts ben... i was supposed to do this ... in http://sourceforge.net/tracker/index.php?func=detail&aid=1958195&group_id=209147&atid=1008544 but looks like this is going to be sufficient. thanks for doing my work :)... (as always) i was surprised to see that readlink is installed by default on cygwin as well. So this should be good enough for cygwin as well. Also, maybe we can add zookeeper.root.logger as a system property which the scripts can set so the lo4j.properties looks like root.Logger = ${zookeeper.root.logger} and make it configurable via the script -Dzookeeper.root.logger=${ZOOKEEPER_ROOT_LOGGER:-INFO,console}" ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-05 23:08 Message: Logged In: YES user_id=972206 Originator: YES Much nicer. Thanks. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-05 21:12 Message: Logged In: YES user_id=154690 Originator: NO Okay, I've enhanced your patch a lot Ted. It assumes an installation directory structure of bin, lib, and conf as siblings. It finds the directory based on the realpath of the script being run. (This is how things like firefox and java are setup on Linux usually.) zkServer.sh is designed to be run from init.d zkCleanup.sh is designed to run from cron zkCli.sh is designed to run from a shell. File Added: scripts.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 22:58 Message: Logged In: YES user_id=972206 Originator: YES Ben, Your comments are good, but this is a place-holder. I would hope that you guys would have something much better soon. a) regarding logging, the logging system should do log rotation and cleanup. Until logging is available in a released version (and I build a log configuration file), I will just do the rotation in this script. It is a placeholder and work-around, after all. b) regarding dirname, please change anything like that you like. It is a good idea. c) regarding zoo.cfg in /etc, I can't comment if that really is a good place or not. If you would like to standardize on that, fine. Perhaps, I should put in a bit of baby logic to look first in /etc/zoo.cfg and then in $ZOO/zoo.cfg. d) regarding zoo.cfg instead of ZOO/zoo.cfg, that is just an error. Thanks for catching it. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 22:00 Message: Logged In: YES user_id=154690 Originator: NO Thanx Ted. I think ZOO should be set using dirname $0 Log cleanup shouldn't really be done in the startup scripts right? Finally, I think we should get the config from /etc/zookeeper/zoo.cfg right? In your script you use $ZOO/zoo.cfg and zoo.cfg, but both seem wrong (with the latter more wrong :) ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 21:20 Message: Logged In: YES user_id=154690 Originator: NO Deleted old version so it was clear what we were reviewing. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-30 17:47 Message: Logged In: YES user_id=972206 Originator: YES Here is an updated script that uses the kill command. I am quite dubious of the wisdom of allowing arbitrary processes anywhere on the net to kill a zookeeper. It definitely means that zookeepers cannot be exposed at all to anything except a very friendly environment. File Added: zookeeper.sh ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-29 23:42 Message: Logged In: YES user_id=972206 Originator: YES The kill hack is interesting. Good thing to know about. Kind of a dangerous feature, though. It also requires that I know something about the config or write something clever in the script that turns the config file into the telnet or nc command. The logs I am talking about are the accumulation of what currently comes out of standard output, not the transactional logs. I think that there is an enhancement coming that will use log4J. That would be vastly better than my hack of putting standard out to a file. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-04-29 22:34 Message: Logged In: YES user_id=1926652 Originator: NO A better way to kill a server to send a kill command to the server's client port (the clientPort parameter in zoo.cfg), for example: echo kill|nc localhost 1234 ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-25 18:52 Message: Logged In: YES user_id=12853 Originator: NO Ted, be aware that there is also the following to be managed. The "log files" reference are transacitonal log files - not logging log files. "The ZooKeeper server creates snapshot and log files, but never deletes them. The retention policy of the data and log files is implemented outside of the ZooKeeper server. The server itself only needs the latest complete fuzzy snapshot and the log files from the start of that snapshot. The PurgeTxnLog utility implements a simple retention policy that administrators can use." http://zookeeper.wiki.sourceforge.net/ZooKeeperDataFormat ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1951806&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 18:39:35
|
Patches item #1951806, was opened at 2008-04-25 10:10 Message generated for change (Comment added) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1951806&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Mahadev Konar (mahadevkonar) Summary: Sample startup script Initial Comment: This may be useful as a temporary script. Eventually there are several things that should be improved: a) the log rotation hack should go away when real logging get implemented b) the kill hack should go away when there is a good way to kill the process. In the meantime, this may be handy to people at least as a starting point. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-07 11:39 Message: Logged In: YES user_id=154690 Originator: NO +1 make it happen ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 15:58 Message: Logged In: YES user_id=1926680 Originator: NO i am uploading a patch that makes log4j configurable in the scripts and also made changes to the script so that it works in the developer environment as well . File Added: log4j_build_script_1.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 12:46 Message: Logged In: YES user_id=12853 Originator: NO +1 but should also include update to the wiki to document: a) the commands b) usage, in particular env variables and default expected locations (for example ZOOLIBDIR is bin/../lib + should have all necessary libs, zoo.cfg + log4j.properties are in ZOOCFGDIR, etc...) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:43 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-05 23:22 Message: Logged In: YES user_id=1926680 Originator: NO nice scripts ben... i was supposed to do this ... in http://sourceforge.net/tracker/index.php?func=detail&aid=1958195&group_id=209147&atid=1008544 but looks like this is going to be sufficient. thanks for doing my work :)... (as always) i was surprised to see that readlink is installed by default on cygwin as well. So this should be good enough for cygwin as well. Also, maybe we can add zookeeper.root.logger as a system property which the scripts can set so the lo4j.properties looks like root.Logger = ${zookeeper.root.logger} and make it configurable via the script -Dzookeeper.root.logger=${ZOOKEEPER_ROOT_LOGGER:-INFO,console}" ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-05 16:08 Message: Logged In: YES user_id=972206 Originator: YES Much nicer. Thanks. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-05 14:12 Message: Logged In: YES user_id=154690 Originator: NO Okay, I've enhanced your patch a lot Ted. It assumes an installation directory structure of bin, lib, and conf as siblings. It finds the directory based on the realpath of the script being run. (This is how things like firefox and java are setup on Linux usually.) zkServer.sh is designed to be run from init.d zkCleanup.sh is designed to run from cron zkCli.sh is designed to run from a shell. File Added: scripts.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 15:58 Message: Logged In: YES user_id=972206 Originator: YES Ben, Your comments are good, but this is a place-holder. I would hope that you guys would have something much better soon. a) regarding logging, the logging system should do log rotation and cleanup. Until logging is available in a released version (and I build a log configuration file), I will just do the rotation in this script. It is a placeholder and work-around, after all. b) regarding dirname, please change anything like that you like. It is a good idea. c) regarding zoo.cfg in /etc, I can't comment if that really is a good place or not. If you would like to standardize on that, fine. Perhaps, I should put in a bit of baby logic to look first in /etc/zoo.cfg and then in $ZOO/zoo.cfg. d) regarding zoo.cfg instead of ZOO/zoo.cfg, that is just an error. Thanks for catching it. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 15:00 Message: Logged In: YES user_id=154690 Originator: NO Thanx Ted. I think ZOO should be set using dirname $0 Log cleanup shouldn't really be done in the startup scripts right? Finally, I think we should get the config from /etc/zookeeper/zoo.cfg right? In your script you use $ZOO/zoo.cfg and zoo.cfg, but both seem wrong (with the latter more wrong :) ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:20 Message: Logged In: YES user_id=154690 Originator: NO Deleted old version so it was clear what we were reviewing. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-30 10:47 Message: Logged In: YES user_id=972206 Originator: YES Here is an updated script that uses the kill command. I am quite dubious of the wisdom of allowing arbitrary processes anywhere on the net to kill a zookeeper. It definitely means that zookeepers cannot be exposed at all to anything except a very friendly environment. File Added: zookeeper.sh ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-29 16:42 Message: Logged In: YES user_id=972206 Originator: YES The kill hack is interesting. Good thing to know about. Kind of a dangerous feature, though. It also requires that I know something about the config or write something clever in the script that turns the config file into the telnet or nc command. The logs I am talking about are the accumulation of what currently comes out of standard output, not the transactional logs. I think that there is an enhancement coming that will use log4J. That would be vastly better than my hack of putting standard out to a file. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-04-29 15:34 Message: Logged In: YES user_id=1926652 Originator: NO A better way to kill a server to send a kill command to the server's client port (the clientPort parameter in zoo.cfg), for example: echo kill|nc localhost 1234 ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-25 11:52 Message: Logged In: YES user_id=12853 Originator: NO Ted, be aware that there is also the following to be managed. The "log files" reference are transacitonal log files - not logging log files. "The ZooKeeper server creates snapshot and log files, but never deletes them. The retention policy of the data and log files is implemented outside of the ZooKeeper server. The server itself only needs the latest complete fuzzy snapshot and the log files from the start of that snapshot. The PurgeTxnLog utility implements a simple retention policy that administrators can use." http://zookeeper.wiki.sourceforge.net/ZooKeeperDataFormat ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1951806&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 18:25:43
|
Patches item #1913998, was opened at 2008-03-13 18:52 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1913998&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 5 Private: No Submitted By: Andrew Kornev (akornev) Assigned to: Nobody/Anonymous (nobody) Summary: JMX instrumentation Initial Comment: This patch adds support for management and monitoring via JMX. It uses the MXBean feature made available in Java 6. The instrumentation code relies on the new lifecycle event notification mechanism to dynamically register and unregister MBeans. The ant build file has NOT been modified to build this new feature yet. There are no javadocs for the new classes either (but this will be fixed, of course). ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-07 11:25 Message: Logged In: YES user_id=1926652 Originator: YES Moved around ManagedZooKeeperServer and ManagedQuorumPeer classes back to the jmx source tree to make it possible to build the src source tree whithout the jmx tree. Now would be a good time to start reviewing this patch. File Added: jmx-3.patch ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-01 18:06 Message: Logged In: YES user_id=1926652 Originator: YES Added javadocs, updated the code to use log4j, moved common classes to the src/ directory tree. File Added: jmx-2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-17 13:44 Message: Logged In: YES user_id=1926680 Originator: NO that is primarily my concern. Meaning if jmx is to be enabled in production then creating an ojbect per connection would deteriorate the scalability of zookeeper. What I mean is that do we actually need per connection management in jmx? or we could have just connection stats in jmx and some other means to debug problems with connections (other than jmx)? ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-03-17 13:14 Message: Logged In: YES user_id=1926652 Originator: YES JMX is primarily for management and monitoring, and my hope is to have JMX enabled in production by default. JMX instrumentation code will eventually rely on the /proc data tree (when it becomes available) to get connection-related information. But we're still gonna have to have a bean per connection registered with MBeanServer. Dynamic registering/unregistering of connection beans may indeed incur some (insignificant) overhead, but I'm not aware of any other way of making a dynamic resource avilable to JMX. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-17 12:50 Message: Logged In: YES user_id=1926680 Originator: NO 3) for this what I meant was that .. we c annot run with jmx on in production .. and if connection tracking is for debugging purpose then jmx just does not sound the right tool for debugging... 4) thats true... but lets have a better process in line :) ... sorry to have started with you. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-03-17 12:23 Message: Logged In: YES user_id=1926652 Originator: YES 1) yes! 2) updated the code to use logError() for logging errors only and logTraceMessage() for logging debug/info. 3) not sure I understand the concern. Connection tracking using /proc FS is orthogonal to JMX. JMX Beans would have to be instantiated for each new connection anyway if we want to be able to manage connections thru JMX. 4) this may take a while. Historically, however, lacking and/or misleading javadocs have never been a showstopper for the existing ZK code ;-) ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-15 11:02 Message: Logged In: YES user_id=1926680 Originator: NO 1) dataTreeBean = new com.yahoo.zookeeper.jmx.server.DataTreeBean( + server.dataTree); can we just replace this with DataTreeBean(datatree)? 2) also i wonder if we should have logging levels on the server side like debug and info , warn, fatal right now we just have logerror and logwarn... and we end up using logError for everything .. 3) am really unconfortable with the idea that we keep track of all the connections using jmx beans? I thought we were going to put all this info in the /proc file system? We end up creating a new object connection bean for every connection (as per my understanding) ... 4) can you upload a patch with javadocs? :) Its easier to review :). ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-03-14 10:11 Message: Logged In: YES user_id=1926652 Originator: YES Are you sure it's this patch that has tabs in it? Or, maybe, you meant the other one (the refactoring patch)? Because that one does have tabs (which I've already fixed in locally). But this one does not. Thanks for reviewing! ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-03-13 19:20 Message: Logged In: YES user_id=1926680 Originator: NO this patch has tabs in it... i havent finished reviewing... just getting my review comments out... ill let you know when I am done fully reviewing the patch so that you dont have to upload it again and again. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1913998&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 05:51:01
|
Patches item #1958274, was opened at 2008-05-05 14:16 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-06 22:44 Message: Logged In: YES user_id=12853 Originator: NO Flavio please delete the old patch(es). ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-06 22:17 Message: Logged In: YES user_id=1926444 Originator: YES File Added: patch-fle.diff ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-06 22:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 19:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 11:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 10:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 05:16:58
|
Patches item #1958274, was opened at 2008-05-05 23:16 Message generated for change (Comment added) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: fpj (fpj) Date: 2008-05-07 07:17 Message: Logged In: YES user_id=1926444 Originator: YES File Added: patch-fle.diff ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-07 07:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 04:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 20:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 05:05:41
|
Patches item #1958274, was opened at 2008-05-05 23:16 Message generated for change (Comment added) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: fpj (fpj) Date: 2008-05-07 07:05 Message: Logged In: YES user_id=1926444 Originator: YES No, they are patches for different implementations. I separated them to make it easier to review the patches. -Flavio ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 04:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 20:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 02:54:11
|
Patches item #1958274, was opened at 2008-05-05 21:16 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: server Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Removed unused variables from FastLeaderElection Initial Comment: Removed unused variables from FastLeaderElection. -Flavio ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-07 02:54 Message: Logged In: YES user_id=1926680 Originator: NO is this related to https://sourceforge.net/tracker/index.php?func=detail&aid=1956801&group_id=209147&atid=1008546 ? from the description it looks like they are the same... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 18:34 Message: Logged In: YES user_id=12853 Originator: NO There is an issue with this patch: 1) line 248 has "rand" variable which the patch removes initialization, but not the field also: 2) cleanup imports (remove random and zoolog) 3) there are more unused vars which could be removed, lines; 152, 250, 269 Btw - if you are using eclipse you can get findbugs at: http://findbugs.sourceforge.net/manual/eclipse.html ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 17:40 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1958274&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-07 01:16:00
|
Patches item #1956801, was opened at 2008-05-03 19:00 Message generated for change (Comment added) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 7 Private: No Submitted By: fpj (fpj) Assigned to: Mahadev Konar (mahadevkonar) Summary: Small fixes to Authenticated LE Initial Comment: * Removed unused variable; * Removed a couple of naked notifies; * Fixed a type cast; * Removed an unnecessary exception catch; * Replaced numerical constructor with valueOf in a couple of places. ---------------------------------------------------------------------- >Comment By: fpj (fpj) Date: 2008-05-07 03:16 Message: Logged In: YES user_id=1926444 Originator: YES Have taken the following actions: * Put back the synchronized blocks; * Have double-checked that the naked notifies are actually ok. Findbugs complains about no change to the state of the object, which is true. The notify is triggered by the reception of a message, though, which is an external event, and that's why there is no state change upon the execution of the notify. File Added: patch-auth-le ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 20:50 Message: Logged In: YES user_id=12853 Originator: NO 1) On line 214/339 synchronization was removed, don't you have to hold the monitor when calling notify? "This method should only be called by a thread that is the owner of this object's monitor." 2) Remove unused field line 377 3) This is a new one to me -- Findbugs has an interesting comment for lines 536, 567 "[DL] Synchronization on boxed primative could lead to deadlock [DL_SYNCHRONIZATION_ON_BOXED_PRIMITIVE] The code synchronizes on a boxed primitive constant, such as an Integer. private static Integer count = 0; ... synchronized(count) { count++; } ... Since Integer objects can be cached and shared, this code could be synchronizing on the same object as other, unrelated code, leading to unresponsiveness and possible deadlock" ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:41 Message: Logged In: YES user_id=12853 Originator: NO The assignee is responsible for closing (committing) this patch (see ZooKeeperPatches on wiki) -- this includes getting additional review if necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956801&group_id=209147 |
