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-12 21:22:58
|
Patches item #1961309, was opened at 2008-05-09 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=1961309&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: fpj (fpj) Assigned to: Andrew Kornev (akornev) Summary: Small fixes to QuorumCnxManager Initial Comment: Small changes to QuorumCnxManager.java: 1) Removed unnecessary null checks; 2) Removed commented code. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-12 14:23 Message: Logged In: YES user_id=1926652 Originator: NO +1 what could possibly go wrong? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961309&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 21:09:33
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Benjamin Reed (breed) Summary: KeeperException should be sub-typed Initial Comment: Note that one of the tests doesn't seem to run for me. This does not look related to this patch. I am having some disk space issues just now that are preventing me from running this test again. This patch turned out a bit bigger than expected because I went back into all of the code and made the construction of KeeperExceptions as specific as possible. At the same time, I trimmed down as many of the throws declarations as I could. This is the second patch in this series. It changes an error message in KeeperException.create to be more clear. Specifically: * The generic constructors for KeeperException were hidden to prevent direct construction. * All direct constructors of KeeperException were hidden in the zookeeper.exceptions package * A factory method was created for constructing sub-classed KeeperExceptions based on error code * All constructions of KeeperException were examined and converted to direct constructions of the specific sub-types or to a call to the factory method * A few instances of catch(KeeperException) were made more specific to avoid the immediately following if-statement * Some cases of redundant catches were removed * Some javadoc was correct vis a vis Exceptions thrown * Some unused imports were removed from TestClient Some gratuitous changes in the order of imports were introduced as an artifact of the semi-automated method used to do these changes. ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-12 14:09 Message: Logged In: YES user_id=12853 Originator: NO sf has been flakey lately. Had a bunch of issues last week trying to update tracker items. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 14:06 Message: Logged In: YES user_id=972206 Originator: YES I definitely missed some updates. I have dealt with the conflicts, but I can't seem to update from SVN right now. It worked 10 minutes ago, but not now. Time for tea. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 13:39 Message: Logged In: YES user_id=972206 Originator: YES I will take a look. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 15:09 Message: Logged In: YES user_id=154690 Originator: NO It looks like this code reverts a change in ClientCnxn. Ted, can you see if you missed an update? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 15:00 Message: Logged In: YES user_id=12853 Originator: NO +1 this looks good to me. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:52 Message: Logged In: YES user_id=154690 Originator: NO +1 I like it! We need to hold this for the start of the 3.0.0 release since it is a slight API break, even though I doubt anyone would notice. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:48 Message: Logged In: YES user_id=972206 Originator: YES Yes. I was going to make them inner. Here is another replacement patch. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=972206 Originator: YES I am sure it is just you that caused me to create a half-patch. :-) Here is a corrected one. I deleted the broken one. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=154690 Originator: NO Yeah I think he for got them. Weren't you going to nest them in KeeperException Ted? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 14:37 Message: Logged In: YES user_id=12853 Originator: NO Just me or is the latest patch missing the com.yahoo.zookeeper.exceptions.* classes/package? ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-30 15:20 Message: Logged In: YES user_id=972206 Originator: YES Here is a cleaner patch. It leaves KeeperException out in the open and makes it abstract. This avoids an API change, but still gets the benefits I was looking for. I also addeda test case to check for messages and sub-class of KE for all known codes. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-23 15:50 Message: Logged In: YES user_id=972206 Originator: YES I will provide another patch that does: a) KeeperException back at the top and made abstract b) All sub-types will be nested under KeeperException c) A test that confirms all codes have messages and all sub-types have the correct code. These three changes will address most of the comments. To wit: - my desire to make sure that access is controlled - Ben's desire to have a cross-check on error codes on the wire vs exception types - Patrick's desire not to have a new package - everybody's desire to avoid API changes I should get this out in a day or two. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 11:00 Message: Logged In: YES user_id=12853 Originator: NO Ok, that sounds reasonable to me. If this is the rational then we should be sure to document it clearly -- at the very least the package.html for exceptions package should make this clear. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-23 10:44 Message: Logged In: YES user_id=154690 Originator: NO In general I agree with you Pat, I think for KeeperException the case is a bit different. Except for corner cases KeeperException codes get translated to an int and flow across wires. So, we need to have a good handle on the possible exceptions and their corresponding code. This makes it useful to centralize things a bit. Putting everything in an exception package in nice in that way: we easily see the possible KeeperExceptions, so we can make sure the codes are assigned correctly, we have messages for each code, KeeperException.create() constructs the correct subclass, etc. I'm not a big fan of nested classes. (Existing code notwithstanding :) But, in this case, perhaps we should make the subclasses of KeeperException nested classes. Each subclass is just one line of code anyway, so it saves some clutter in the sourcetree. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 09:23 Message: Logged In: YES user_id=12853 Originator: NO I agree that there's not alot of cases, probably even none. Perhaps I'm just stuck on what I consider "std idiom" for exceptions in java. My personal preference is to mimic java APIs. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 17:51 Message: Logged In: YES user_id=972206 Originator: YES Patrick I don't think it is a problem to have the exceptions near the code that they come from. I do think there are vanishingly few (== none?) use cases for a generic KeeperException. Since a code is required and it is part of the contract that every valid code have a specific sub-class, then any KeeperException should be a sub-type. Perhaps a better way to enforce that is to just make KeeperException abstract. If there is consensus to do so, I can move the exceptions back to the top level pretty easily. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-22 17:22 Message: Logged In: YES user_id=12853 Originator: NO Hi Ted, I like this change, it also struck me as odd to not have sub-types. However I find pushing the exceptions into their own package unnecessary. I realize the idea is to hide the constructor of KeeperException, however I don't think this is required. We can document the constructor of KeeperException and state that creators should use a more specific specific subclass. In some cases it may even be necessary for someone to instantiate a generic "KeeperException" (not a sub). I see this analogous to java.lang.Exception and it's subclasses. It can be abused, but that can be weeded out during patch review. I'd rather that the exceptions live "intermingled" with the non-exception classes. If QuorumPeer needs to throw a QuorumException should be in quorum package, no? (if there was such a thing). This is similar to InputStream throwing a IOException or one of it's more specific sub-types. Regards, Patrick ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 21:06:03
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Comment added) made by tedunning You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Benjamin Reed (breed) Summary: KeeperException should be sub-typed Initial Comment: Note that one of the tests doesn't seem to run for me. This does not look related to this patch. I am having some disk space issues just now that are preventing me from running this test again. This patch turned out a bit bigger than expected because I went back into all of the code and made the construction of KeeperExceptions as specific as possible. At the same time, I trimmed down as many of the throws declarations as I could. This is the second patch in this series. It changes an error message in KeeperException.create to be more clear. Specifically: * The generic constructors for KeeperException were hidden to prevent direct construction. * All direct constructors of KeeperException were hidden in the zookeeper.exceptions package * A factory method was created for constructing sub-classed KeeperExceptions based on error code * All constructions of KeeperException were examined and converted to direct constructions of the specific sub-types or to a call to the factory method * A few instances of catch(KeeperException) were made more specific to avoid the immediately following if-statement * Some cases of redundant catches were removed * Some javadoc was correct vis a vis Exceptions thrown * Some unused imports were removed from TestClient Some gratuitous changes in the order of imports were introduced as an artifact of the semi-automated method used to do these changes. ---------------------------------------------------------------------- >Comment By: Ted Dunning (tedunning) Date: 2008-05-12 14:06 Message: Logged In: YES user_id=972206 Originator: YES I definitely missed some updates. I have dealt with the conflicts, but I can't seem to update from SVN right now. It worked 10 minutes ago, but not now. Time for tea. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 13:39 Message: Logged In: YES user_id=972206 Originator: YES I will take a look. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 15:09 Message: Logged In: YES user_id=154690 Originator: NO It looks like this code reverts a change in ClientCnxn. Ted, can you see if you missed an update? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 15:00 Message: Logged In: YES user_id=12853 Originator: NO +1 this looks good to me. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:52 Message: Logged In: YES user_id=154690 Originator: NO +1 I like it! We need to hold this for the start of the 3.0.0 release since it is a slight API break, even though I doubt anyone would notice. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:48 Message: Logged In: YES user_id=972206 Originator: YES Yes. I was going to make them inner. Here is another replacement patch. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=972206 Originator: YES I am sure it is just you that caused me to create a half-patch. :-) Here is a corrected one. I deleted the broken one. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=154690 Originator: NO Yeah I think he for got them. Weren't you going to nest them in KeeperException Ted? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 14:37 Message: Logged In: YES user_id=12853 Originator: NO Just me or is the latest patch missing the com.yahoo.zookeeper.exceptions.* classes/package? ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-30 15:20 Message: Logged In: YES user_id=972206 Originator: YES Here is a cleaner patch. It leaves KeeperException out in the open and makes it abstract. This avoids an API change, but still gets the benefits I was looking for. I also addeda test case to check for messages and sub-class of KE for all known codes. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-23 15:50 Message: Logged In: YES user_id=972206 Originator: YES I will provide another patch that does: a) KeeperException back at the top and made abstract b) All sub-types will be nested under KeeperException c) A test that confirms all codes have messages and all sub-types have the correct code. These three changes will address most of the comments. To wit: - my desire to make sure that access is controlled - Ben's desire to have a cross-check on error codes on the wire vs exception types - Patrick's desire not to have a new package - everybody's desire to avoid API changes I should get this out in a day or two. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 11:00 Message: Logged In: YES user_id=12853 Originator: NO Ok, that sounds reasonable to me. If this is the rational then we should be sure to document it clearly -- at the very least the package.html for exceptions package should make this clear. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-23 10:44 Message: Logged In: YES user_id=154690 Originator: NO In general I agree with you Pat, I think for KeeperException the case is a bit different. Except for corner cases KeeperException codes get translated to an int and flow across wires. So, we need to have a good handle on the possible exceptions and their corresponding code. This makes it useful to centralize things a bit. Putting everything in an exception package in nice in that way: we easily see the possible KeeperExceptions, so we can make sure the codes are assigned correctly, we have messages for each code, KeeperException.create() constructs the correct subclass, etc. I'm not a big fan of nested classes. (Existing code notwithstanding :) But, in this case, perhaps we should make the subclasses of KeeperException nested classes. Each subclass is just one line of code anyway, so it saves some clutter in the sourcetree. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 09:23 Message: Logged In: YES user_id=12853 Originator: NO I agree that there's not alot of cases, probably even none. Perhaps I'm just stuck on what I consider "std idiom" for exceptions in java. My personal preference is to mimic java APIs. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 17:51 Message: Logged In: YES user_id=972206 Originator: YES Patrick I don't think it is a problem to have the exceptions near the code that they come from. I do think there are vanishingly few (== none?) use cases for a generic KeeperException. Since a code is required and it is part of the contract that every valid code have a specific sub-class, then any KeeperException should be a sub-type. Perhaps a better way to enforce that is to just make KeeperException abstract. If there is consensus to do so, I can move the exceptions back to the top level pretty easily. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-22 17:22 Message: Logged In: YES user_id=12853 Originator: NO Hi Ted, I like this change, it also struck me as odd to not have sub-types. However I find pushing the exceptions into their own package unnecessary. I realize the idea is to hide the constructor of KeeperException, however I don't think this is required. We can document the constructor of KeeperException and state that creators should use a more specific specific subclass. In some cases it may even be necessary for someone to instantiate a generic "KeeperException" (not a sub). I see this analogous to java.lang.Exception and it's subclasses. It can be abused, but that can be weeded out during patch review. I'd rather that the exceptions live "intermingled" with the non-exception classes. If QuorumPeer needs to throw a QuorumException should be in quorum package, no? (if there was such a thing). This is similar to InputStream throwing a IOException or one of it's more specific sub-types. Regards, Patrick ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:39:23
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Comment added) made by tedunning You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Benjamin Reed (breed) Summary: KeeperException should be sub-typed Initial Comment: Note that one of the tests doesn't seem to run for me. This does not look related to this patch. I am having some disk space issues just now that are preventing me from running this test again. This patch turned out a bit bigger than expected because I went back into all of the code and made the construction of KeeperExceptions as specific as possible. At the same time, I trimmed down as many of the throws declarations as I could. This is the second patch in this series. It changes an error message in KeeperException.create to be more clear. Specifically: * The generic constructors for KeeperException were hidden to prevent direct construction. * All direct constructors of KeeperException were hidden in the zookeeper.exceptions package * A factory method was created for constructing sub-classed KeeperExceptions based on error code * All constructions of KeeperException were examined and converted to direct constructions of the specific sub-types or to a call to the factory method * A few instances of catch(KeeperException) were made more specific to avoid the immediately following if-statement * Some cases of redundant catches were removed * Some javadoc was correct vis a vis Exceptions thrown * Some unused imports were removed from TestClient Some gratuitous changes in the order of imports were introduced as an artifact of the semi-automated method used to do these changes. ---------------------------------------------------------------------- >Comment By: Ted Dunning (tedunning) Date: 2008-05-12 13:39 Message: Logged In: YES user_id=972206 Originator: YES I will take a look. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 15:09 Message: Logged In: YES user_id=154690 Originator: NO It looks like this code reverts a change in ClientCnxn. Ted, can you see if you missed an update? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 15:00 Message: Logged In: YES user_id=12853 Originator: NO +1 this looks good to me. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:52 Message: Logged In: YES user_id=154690 Originator: NO +1 I like it! We need to hold this for the start of the 3.0.0 release since it is a slight API break, even though I doubt anyone would notice. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:48 Message: Logged In: YES user_id=972206 Originator: YES Yes. I was going to make them inner. Here is another replacement patch. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=972206 Originator: YES I am sure it is just you that caused me to create a half-patch. :-) Here is a corrected one. I deleted the broken one. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=154690 Originator: NO Yeah I think he for got them. Weren't you going to nest them in KeeperException Ted? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 14:37 Message: Logged In: YES user_id=12853 Originator: NO Just me or is the latest patch missing the com.yahoo.zookeeper.exceptions.* classes/package? ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-30 15:20 Message: Logged In: YES user_id=972206 Originator: YES Here is a cleaner patch. It leaves KeeperException out in the open and makes it abstract. This avoids an API change, but still gets the benefits I was looking for. I also addeda test case to check for messages and sub-class of KE for all known codes. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-23 15:50 Message: Logged In: YES user_id=972206 Originator: YES I will provide another patch that does: a) KeeperException back at the top and made abstract b) All sub-types will be nested under KeeperException c) A test that confirms all codes have messages and all sub-types have the correct code. These three changes will address most of the comments. To wit: - my desire to make sure that access is controlled - Ben's desire to have a cross-check on error codes on the wire vs exception types - Patrick's desire not to have a new package - everybody's desire to avoid API changes I should get this out in a day or two. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 11:00 Message: Logged In: YES user_id=12853 Originator: NO Ok, that sounds reasonable to me. If this is the rational then we should be sure to document it clearly -- at the very least the package.html for exceptions package should make this clear. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-23 10:44 Message: Logged In: YES user_id=154690 Originator: NO In general I agree with you Pat, I think for KeeperException the case is a bit different. Except for corner cases KeeperException codes get translated to an int and flow across wires. So, we need to have a good handle on the possible exceptions and their corresponding code. This makes it useful to centralize things a bit. Putting everything in an exception package in nice in that way: we easily see the possible KeeperExceptions, so we can make sure the codes are assigned correctly, we have messages for each code, KeeperException.create() constructs the correct subclass, etc. I'm not a big fan of nested classes. (Existing code notwithstanding :) But, in this case, perhaps we should make the subclasses of KeeperException nested classes. Each subclass is just one line of code anyway, so it saves some clutter in the sourcetree. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 09:23 Message: Logged In: YES user_id=12853 Originator: NO I agree that there's not alot of cases, probably even none. Perhaps I'm just stuck on what I consider "std idiom" for exceptions in java. My personal preference is to mimic java APIs. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 17:51 Message: Logged In: YES user_id=972206 Originator: YES Patrick I don't think it is a problem to have the exceptions near the code that they come from. I do think there are vanishingly few (== none?) use cases for a generic KeeperException. Since a code is required and it is part of the contract that every valid code have a specific sub-class, then any KeeperException should be a sub-type. Perhaps a better way to enforce that is to just make KeeperException abstract. If there is consensus to do so, I can move the exceptions back to the top level pretty easily. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-22 17:22 Message: Logged In: YES user_id=12853 Originator: NO Hi Ted, I like this change, it also struck me as odd to not have sub-types. However I find pushing the exceptions into their own package unnecessary. I realize the idea is to hide the constructor of KeeperException, however I don't think this is required. We can document the constructor of KeeperException and state that creators should use a more specific specific subclass. In some cases it may even be necessary for someone to instantiate a generic "KeeperException" (not a sub). I see this analogous to java.lang.Exception and it's subclasses. It can be abused, but that can be weeded out during patch review. I'd rather that the exceptions live "intermingled" with the non-exception classes. If QuorumPeer needs to throw a QuorumException should be in quorum package, no? (if there was such a thing). This is similar to InputStream throwing a IOException or one of it's more specific sub-types. Regards, Patrick ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:18:22
|
Patches item #1913998, was opened at 2008-03-13 18:52 Message generated for change (Settings changed) made by breed 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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Andrew Kornev (akornev) >Assigned to: Mahadev Konar (mahadevkonar) 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-12 20:16:40
|
Patches item #1944401, was opened at 2008-04-16 14:43 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944401&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Patrick Hunt (phunt) >Assigned to: Mahadev Konar (mahadevkonar) Summary: make callbacks more abstract Initial Comment: Replace arraylist with list in acl and children callbacks. Will this effect ppl upgrading? Probably should mitigate by release noting. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:36 Message: Logged In: YES user_id=154690 Originator: NO We should hold until 3.0.0 because of the API change. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:36 Message: Logged In: YES user_id=154690 Originator: NO +1 patch looks good ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-16 15:01 Message: Logged In: YES user_id=154690 Originator: NO Yes, it will effect existing code. I think we should hold this patch until we do the next release. Then I'd like to put in this patch, fix the bug that changes the watch types and states to enums instead of int, and put in the safety checks in the feature requests. I'm wondering if this patch should create an async or callback package and make the interfaces top level interfaces in that package rather than using the AsyncCallback container. I don't mind how it is now, but Ted commented that some IDEs have problems with nested classes. On the other hand, it will break the implementors of this interface even more. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944401&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:16:27
|
Patches item #1944441, was opened at 2008-04-16 16:01 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944441&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Patrick Hunt (phunt) >Assigned to: Mahadev Konar (mahadevkonar) Summary: fix 1941109 -- arraylist -> list<T> in interfaces Initial Comment: fix for 1941109 -- move to more abstract interfaces. arraylist -> list<t> This may require changes to user code. Documentation will need to be updated. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:30 Message: Logged In: YES user_id=154690 Originator: NO We need to hold this until we start 3.0.0 because of the API breakage. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:30 Message: Logged In: YES user_id=154690 Originator: NO +1 patch looks good ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-17 09:56 Message: Logged In: YES user_id=154690 Originator: NO Can we merge this with 1944401. The purposes overlap. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944441&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:16:13
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ted Dunning (tedunning) >Assigned to: Benjamin Reed (breed) Summary: KeeperException should be sub-typed Initial Comment: Note that one of the tests doesn't seem to run for me. This does not look related to this patch. I am having some disk space issues just now that are preventing me from running this test again. This patch turned out a bit bigger than expected because I went back into all of the code and made the construction of KeeperExceptions as specific as possible. At the same time, I trimmed down as many of the throws declarations as I could. This is the second patch in this series. It changes an error message in KeeperException.create to be more clear. Specifically: * The generic constructors for KeeperException were hidden to prevent direct construction. * All direct constructors of KeeperException were hidden in the zookeeper.exceptions package * A factory method was created for constructing sub-classed KeeperExceptions based on error code * All constructions of KeeperException were examined and converted to direct constructions of the specific sub-types or to a call to the factory method * A few instances of catch(KeeperException) were made more specific to avoid the immediately following if-statement * Some cases of redundant catches were removed * Some javadoc was correct vis a vis Exceptions thrown * Some unused imports were removed from TestClient Some gratuitous changes in the order of imports were introduced as an artifact of the semi-automated method used to do these changes. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 15:09 Message: Logged In: YES user_id=154690 Originator: NO It looks like this code reverts a change in ClientCnxn. Ted, can you see if you missed an update? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 15:00 Message: Logged In: YES user_id=12853 Originator: NO +1 this looks good to me. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:52 Message: Logged In: YES user_id=154690 Originator: NO +1 I like it! We need to hold this for the start of the 3.0.0 release since it is a slight API break, even though I doubt anyone would notice. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:48 Message: Logged In: YES user_id=972206 Originator: YES Yes. I was going to make them inner. Here is another replacement patch. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=972206 Originator: YES I am sure it is just you that caused me to create a half-patch. :-) Here is a corrected one. I deleted the broken one. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 14:42 Message: Logged In: YES user_id=154690 Originator: NO Yeah I think he for got them. Weren't you going to nest them in KeeperException Ted? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-01 14:37 Message: Logged In: YES user_id=12853 Originator: NO Just me or is the latest patch missing the com.yahoo.zookeeper.exceptions.* classes/package? ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-30 15:20 Message: Logged In: YES user_id=972206 Originator: YES Here is a cleaner patch. It leaves KeeperException out in the open and makes it abstract. This avoids an API change, but still gets the benefits I was looking for. I also addeda test case to check for messages and sub-class of KE for all known codes. File Added: 1948141-c.patch ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-23 15:50 Message: Logged In: YES user_id=972206 Originator: YES I will provide another patch that does: a) KeeperException back at the top and made abstract b) All sub-types will be nested under KeeperException c) A test that confirms all codes have messages and all sub-types have the correct code. These three changes will address most of the comments. To wit: - my desire to make sure that access is controlled - Ben's desire to have a cross-check on error codes on the wire vs exception types - Patrick's desire not to have a new package - everybody's desire to avoid API changes I should get this out in a day or two. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 11:00 Message: Logged In: YES user_id=12853 Originator: NO Ok, that sounds reasonable to me. If this is the rational then we should be sure to document it clearly -- at the very least the package.html for exceptions package should make this clear. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-23 10:44 Message: Logged In: YES user_id=154690 Originator: NO In general I agree with you Pat, I think for KeeperException the case is a bit different. Except for corner cases KeeperException codes get translated to an int and flow across wires. So, we need to have a good handle on the possible exceptions and their corresponding code. This makes it useful to centralize things a bit. Putting everything in an exception package in nice in that way: we easily see the possible KeeperExceptions, so we can make sure the codes are assigned correctly, we have messages for each code, KeeperException.create() constructs the correct subclass, etc. I'm not a big fan of nested classes. (Existing code notwithstanding :) But, in this case, perhaps we should make the subclasses of KeeperException nested classes. Each subclass is just one line of code anyway, so it saves some clutter in the sourcetree. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-23 09:23 Message: Logged In: YES user_id=12853 Originator: NO I agree that there's not alot of cases, probably even none. Perhaps I'm just stuck on what I consider "std idiom" for exceptions in java. My personal preference is to mimic java APIs. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 17:51 Message: Logged In: YES user_id=972206 Originator: YES Patrick I don't think it is a problem to have the exceptions near the code that they come from. I do think there are vanishingly few (== none?) use cases for a generic KeeperException. Since a code is required and it is part of the contract that every valid code have a specific sub-class, then any KeeperException should be a sub-type. Perhaps a better way to enforce that is to just make KeeperException abstract. If there is consensus to do so, I can move the exceptions back to the top level pretty easily. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-04-22 17:22 Message: Logged In: YES user_id=12853 Originator: NO Hi Ted, I like this change, it also struck me as odd to not have sub-types. However I find pushing the exceptions into their own package unnecessary. I realize the idea is to hide the constructor of KeeperException, however I don't think this is required. We can document the constructor of KeeperException and state that creators should use a more specific specific subclass. In some cases it may even be necessary for someone to instantiate a generic "KeeperException" (not a sub). I see this analogous to java.lang.Exception and it's subclasses. It can be abused, but that can be weeded out during patch review. I'd rather that the exceptions live "intermingled" with the non-exception classes. If QuorumPeer needs to throw a QuorumException should be in quorum package, no? (if there was such a thing). This is similar to InputStream throwing a IOException or one of it's more specific sub-types. Regards, Patrick ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1949196&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:15:58
|
Patches item #1961309, was opened at 2008-05-09 18:52 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961309&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: fpj (fpj) >Assigned to: Andrew Kornev (akornev) Summary: Small fixes to QuorumCnxManager Initial Comment: Small changes to QuorumCnxManager.java: 1) Removed unnecessary null checks; 2) Removed commented code. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961309&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:15:19
|
Patches item #1961779, was opened at 2008-05-10 23:17 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961779&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Patrick Hunt (phunt) >Assigned to: Andrew Kornev (akornev) Summary: fix test target in build - handle failure properly Initial Comment: this fixes the build.xml test targets to properly handle failure (fail the whole build). Seems that antcall hides failures in the target. Addresses a problem in the CI integration. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961779&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:06:32
|
Bugs item #1831408, was opened at 2007-11-13 14:43 Message generated for change (Comment added) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1831408&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: java client >Group: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Benjamin Reed (breed) >Assigned to: Mahadev Konar (mahadevkonar) Summary: Use enums rather than ints for types and state Initial Comment: There are a couple of places in ZooKeeper that ints are used when we should really be using enums. The most severe example are the types and states for watch events. It is not always clear which is a type and which is a state. ---------------------------------------------------------------------- >Comment By: Benjamin Reed (breed) Date: 2008-05-12 13:06 Message: Logged In: YES user_id=154690 Originator: YES Bart had expressed intrest in fixing this. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-03-18 08:11 Message: Logged In: YES user_id=154690 Originator: YES Yes please! Contributions are welcome! thanx ben ---------------------------------------------------------------------- Comment By: Don Pinto (dodil) Date: 2008-03-17 21:03 Message: Logged In: YES user_id=1289601 Originator: NO Hi Benjamin, Is it OK for me to work on this ? I am interested to contribute to this project. Thx, Don ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1831408&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:05:53
|
Bugs item #1886743, was opened at 2008-02-04 20:37 Message generated for change (Comment added) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1886743&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Nobody/Anonymous (nobody) >Assigned to: Mahadev Konar (mahadevkonar) Summary: Stat enchaned to include num of children and size Initial Comment: Enhance Stat returned to include number of children and size of data in node. This is make getting this data much cheaper to implement a fuse module. One round-trip is needed instead of two. The only way to get size now is to call get the node data. The only way to get number of children is to call get children. Being able to get this data via node exists would be nice. ---------------------------------------------------------------------- >Comment By: Benjamin Reed (breed) Date: 2008-05-12 13:06 Message: Logged In: YES user_id=154690 Originator: NO To fix this we will need two Stat structures: one for the protocol which will include the above information and one for the ondisk/inmemory structure which will not include that information. (We already have the number of children and the size in memory and we don't want to use up extra bytes to store it redundantly.) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1886743&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:03:18
|
Bugs item #1941109, was opened at 2008-04-12 15:46 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1941109&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: java client >Group: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Nobody/Anonymous (nobody) Assigned to: Patrick Hunt (phunt) Summary: ArrayList is used instead of List Initial Comment: Especially in the stuff to do with ACL's, the return type is often ArrayList instead of List. This makes lots of code much more complex than necessary. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1941109&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:02:01
|
Bugs item #1946934, was opened at 2008-04-19 17:36 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1946934&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ted Dunning (tedunning) >Assigned to: Mahadev Konar (mahadevkonar) Summary: NPE during normal operations Initial Comment: Somehow, I got my server to do this evil thing. I will try to figure out how to do this more consistently. 4/19/08 5:30:31 PM PDT [FinalRequestProcessor.java@220][8]: ****************************** 11944c2037100d8 256 fffffffffffffffe txn type = unknown getChildren n/a 4/19/08 5:30:31 PM PDT [FinalRequestProcessor.java@227][8]: ffffffff0 4/19/08 5:30:31 PM PDT [FinalRequestProcessor.java@228][8]: : java.lang.NullPointerException at java.util.concurrent.ConcurrentHashMap.hash(ConcurrentHashMap.java:157) at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:730) at com.yahoo.zookeeper.server.DataTree.getNode(DataTree.java:84) at com.yahoo.zookeeper.server.FinalRequestProcessor.processRequest(FinalRequestProcessor.java:205) at com.yahoo.zookeeper.server.SyncRequestProcessor.flush(SyncRequestProcessor.java:226) at com.yahoo.zookeeper.server.SyncRequestProcessor.run(SyncRequestProcessor.java:113) 4/19/08 5:30:49 PM PDT [FinalRequestProcessor.java@220][8]: ****************************** 11944c2037100d8 257 fffffffffffffffe txn type = unknown exists n/a 4/19/08 5:30:49 PM PDT [FinalRequestProcessor.java@227][8]: ffffffff0 4/19/08 5:30:49 PM PDT [FinalRequestProcessor.java@228][8]: : java.lang.NullPointerException at com.yahoo.zookeeper.server.FinalRequestProcessor.processRequest(FinalRequestProcessor.java:169) at com.yahoo.zookeeper.server.SyncRequestProcessor.flush(SyncRequestProcessor.java:226) at com.yahoo.zookeeper.server.SyncRequestProcessor.run(SyncRequestProcessor.java:113) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 16:03 Message: Logged In: YES user_id=12853 Originator: NO Ben looks like you had a handle on this. Please submit a patch. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 10:58 Message: Logged In: YES user_id=972206 Originator: YES Good thing then to have found it. My only problem is that by inspection, my code "can't" be sending the null. The usual software engineering definition of "can't" is obviously the one that I mean here. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-22 08:34 Message: Logged In: YES user_id=154690 Originator: NO I think your suspicion is correct. (I thought we were handling this.) It's actually a double bug! Both the server and client should check for a null path. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-21 10:35 Message: Logged In: YES user_id=972206 Originator: YES java version "1.5.0_13" Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_13-b05-241) Java HotSpot(TM) Client VM (build 1.5.0_13-121, mixed mode, sharing) Running on a mac under OSX 10.4 I think actually that this is actually a case of invalid arguments (from me) causing an obscure message rather than being caught. My suspicion is null path passed to create. I still owe you a test case. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-21 09:55 Message: Logged In: YES user_id=154690 Originator: NO Which version of Java are you using? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1946934&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:01:28
|
Bugs item #1948141, was opened at 2008-04-21 11:02 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1948141&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Benjamin Reed (breed) Summary: KeeperException should be sub-classed Initial Comment: The current practice of setting an error code in KeeperException leads to error prone code that looks like this when only one kind of exception can be handled: void createOrUpdate(path, String data) { try { create(path, data) } catch (com.yahoo.zookeeper.KeeperException e) { if (e.getCode() == com.yahoo.zookeeper.KeeperException.Code.NodeExists) { update(path, data, -1) } else { throw e; } } } (ignore the lack of semi's... this is groovy) It would be much better to be able to write: void createOrUpdate(path, String data) { try { create(path, data) } catch (com.yahoo.zookeeper.NodeExistsException e) { update(path, data, -1) } } Where NodeExistsException extends KeeperException. The getCode method can be implemented as before so that old code will not notice the change. This change makes the exception handling much more Java friendly and less like C. I can create a patch if desired. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 16:00 Message: Logged In: YES user_id=12853 Originator: NO Ben is working this out on patch 1949196 with Ted, assigning to Ben. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-22 13:51 Message: Logged In: YES user_id=154690 Originator: NO Fantastic work Ted! Do you mind submitting the patch to the patch tracker? That way we can tie it to a release. We are going to do one more release and then we will be putting in some API breaking patches. They will all be slight breaks, like the one here (changing a package, using an enum instead of int, etc...). My one other nit pick would be that in create() when you throw IllegalArgumentException() you put the error code number in the message. ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 11:46 Message: Logged In: YES user_id=972206 Originator: YES One thing that I should mention is that this patch fixes a minor bug in KeeperException where not all codes had an associated message. I added a message for NotEmpty ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-04-22 11:45 Message: Logged In: YES user_id=972206 Originator: YES Here is a patch. See the patch comments for details. File Added: 1948141.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-22 08:30 Message: Logged In: YES user_id=154690 Originator: NO +1 Excellent suggestion! A patch would be great. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1948141&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:01:08
|
Bugs item #1951802, was opened at 2008-04-25 10:07 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1951802&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: 5 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Benjamin Reed (breed) Summary: Need a script for /etc/init.d Initial Comment: Running zookeeper as a service on a linux machine requires that there be a startup script. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-12 13:00 Message: Logged In: YES user_id=154690 Originator: NO Fixed in [ 1951806 ] Sample startup script ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 15:53 Message: Logged In: YES user_id=12853 Originator: NO Ben is working on this with Ted for patch 1951806 ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1951802&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 20:00:53
|
Bugs item #1951802, was opened at 2008-04-25 10:07 Message generated for change (Comment added) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1951802&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: 5 Private: No Submitted By: Ted Dunning (tedunning) Assigned to: Benjamin Reed (breed) Summary: Need a script for /etc/init.d Initial Comment: Running zookeeper as a service on a linux machine requires that there be a startup script. ---------------------------------------------------------------------- >Comment By: Benjamin Reed (breed) Date: 2008-05-12 13:00 Message: Logged In: YES user_id=154690 Originator: NO Fixed in [ 1951806 ] Sample startup script ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 15:53 Message: Logged In: YES user_id=12853 Originator: NO Ben is working on this with Ted for patch 1951806 ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1951802&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 19:59:46
|
Bugs item #1959060, was opened at 2008-05-06 13:58 Message generated for change (Settings changed) made by breed You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1959060&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: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Patrick Hunt (phunt) Summary: handle failure better in build.xml:test Initial Comment: junit tests should look more like the following which will allow tests to run and build to fail (but all tests will run). <junit haltonfailure="no" fork="yes" errorProperty="tests.failed" failureProperty="tests.failed" ...> ... </junit> <fail if="tests.failed">Tests failed!</fail> ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1959060&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-11 06:17:26
|
Patches item #1961779, was opened at 2008-05-10 23:17 Message generated for change (Tracker Item Submitted) made by Item Submitter You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961779&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: 5 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Nobody/Anonymous (nobody) Summary: fix test target in build - handle failure properly Initial Comment: this fixes the build.xml test targets to properly handle failure (fail the whole build). Seems that antcall hides failures in the target. Addresses a problem in the CI integration. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961779&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-10 01:52:41
|
Patches item #1961309, was opened at 2008-05-10 03:52 Message generated for change (Tracker Item Submitted) made by Item Submitter You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961309&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: 5 Private: No Submitted By: fpj (fpj) Assigned to: Nobody/Anonymous (nobody) Summary: Small fixes to QuorumCnxManager Initial Comment: Small changes to QuorumCnxManager.java: 1) Removed unnecessary null checks; 2) Removed commented code. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961309&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-10 01:15:15
|
Patches item #1955134, was opened at 2008-04-30 23:35 Message generated for change (Settings changed) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1955134&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: None Priority: 7 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: fpj (fpj) Summary: Cleanup logging priorities Initial Comment: This patch cleans up logging priorities (error/warn/info) Previous to log4j all logging was either error or warn (there was no info). I have reviewed all logging looking for: 1) errors that should be warnings I consider a log to be a warning if the code is not exiting, rethrowing an exception or otherwise attempts to recover from the issue. This is not obvious in all cases but generally true. 2) warnings that should be info there was no "info" equivalent in the pre-log4j code. most cases coders used warning level instead. I converted these calls to info where appropriate. 3) I also cleaned up some issues - like missing message in a log call such as LOG.error/warn("MISSINGMSG", e) - ie handling exception. 4) I added a few INFO level messages where appropriate. I suggest that you look for missing messages (esp info) when reviewing the code (submit a patch). ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-10 02:53 Message: Logged In: YES user_id=1926444 Originator: NO Looks good, proceeding to commit. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:44 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. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 23:35 Message: Logged In: YES user_id=154690 Originator: NO +1 make it happen ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 23:34 Message: Logged In: YES user_id=154690 Originator: NO Fixed the patch to make errors that cause System.exit fatal File Added: log4jcleanup(2).patch ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1955134&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-10 01:15:02
|
Patches item #1956509, was opened at 2008-05-03 02:18 Message generated for change (Settings changed) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956509&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: None Priority: 7 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: fpj (fpj) Summary: replace printStackTrace with logging Initial Comment: In some cases printStackTrace as used instead of logging. I updated the code to log appropriately with log4j. ---------------------------------------------------------------------- Comment By: fpj (fpj) Date: 2008-05-10 01:41 Message: Logged In: YES user_id=1926444 Originator: NO All good with this patch. Proceeding to commit it... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:42 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=1956509&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-10 00:53:29
|
Patches item #1955134, was opened at 2008-04-30 23:35 Message generated for change (Comment added) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1955134&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: fpj (fpj) Summary: Cleanup logging priorities Initial Comment: This patch cleans up logging priorities (error/warn/info) Previous to log4j all logging was either error or warn (there was no info). I have reviewed all logging looking for: 1) errors that should be warnings I consider a log to be a warning if the code is not exiting, rethrowing an exception or otherwise attempts to recover from the issue. This is not obvious in all cases but generally true. 2) warnings that should be info there was no "info" equivalent in the pre-log4j code. most cases coders used warning level instead. I converted these calls to info where appropriate. 3) I also cleaned up some issues - like missing message in a log call such as LOG.error/warn("MISSINGMSG", e) - ie handling exception. 4) I added a few INFO level messages where appropriate. I suggest that you look for missing messages (esp info) when reviewing the code (submit a patch). ---------------------------------------------------------------------- >Comment By: fpj (fpj) Date: 2008-05-10 02:53 Message: Logged In: YES user_id=1926444 Originator: NO Looks good, proceeding to commit. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:44 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. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 23:35 Message: Logged In: YES user_id=154690 Originator: NO +1 make it happen ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 23:34 Message: Logged In: YES user_id=154690 Originator: NO Fixed the patch to make errors that cause System.exit fatal File Added: log4jcleanup(2).patch ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1955134&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-09 23:41:10
|
Patches item #1956509, was opened at 2008-05-03 02:18 Message generated for change (Comment added) made by fpj You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1956509&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: fpj (fpj) Summary: replace printStackTrace with logging Initial Comment: In some cases printStackTrace as used instead of logging. I updated the code to log appropriately with log4j. ---------------------------------------------------------------------- >Comment By: fpj (fpj) Date: 2008-05-10 01:41 Message: Logged In: YES user_id=1926444 Originator: NO All good with this patch. Proceeding to commit it... ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-06 19:42 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=1956509&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-09 21:09:15
|
Bugs item #1958183, was opened at 2008-05-05 12:22 Message generated for change (Comment added) made by akornev You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1958183&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: Accepted Priority: 5 Private: No Submitted By: Erik Hetzner (egh) Assigned to: Andrew Kornev (akornev) Summary: tar.gz does not build ; relies on revision number Initial Comment: If ant is run on code from .tar.gz distribution, the version-info task does not do what is supposed to do (generate a Info.java) because the revision number is empty. Additionally, the version-info task does not fail, it continues on its way, so the notification that something is wrong is not known until the build fails due to a missing Info.java file. Additionally, the VerGen class gives us the following error message: All version-related parameters must be invalid integers! I think that valid is the word you want here. :) ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-09 14:09 Message: Logged In: YES user_id=1926652 Originator: NO Fixed (see patch [1956499]) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 15:59 Message: Logged In: YES user_id=12853 Originator: NO Andrew mentioned that he's going to address this issue as part of the dist packaging change. Reassigning to him. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 15:49 Message: Logged In: YES user_id=12853 Originator: NO I noticed the same issue when using GIT to manage local repositories of ZooKeeper. The build fails because it assumes that the code is contained within a svn repository. I will look into resolving this (specifically version gen being tied to svn, not the other distro issues). ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-05 12:38 Message: Logged In: YES user_id=1926652 Originator: NO Erik, thanks for catching that. I think we released the source tar a bit prematurely. While we're working on the fix, you're advised to either use the binary server tar or build from the SVN repository. Thanks! ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1958183&group_id=209147 |
