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-20 00:05:55
|
Patches item #1944441, was opened at 2008-04-16 16:01 Message generated for change (Comment added) made by akornev 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: Fixed 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: Andrew Kornev (akornev) Date: 2008-05-19 17:06 Message: Logged In: YES user_id=1926652 Originator: NO Yes, I mean DataTree.getChildren(). Although, I'm not sure what you meant by "the method is private to zk implementation" (it seems quite public to me), I just thought that if we have changed getACL()'s return type from ArrayList<> to List<> then, for consistency, we should change getChildren()'s, too. Is it possible to fix this as part of this patch? ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-19 16:07 Message: Logged In: YES user_id=12853 Originator: YES You mean DataTree.java right? I think it's ok in this case. The method is private to zk implementation -- I updated the public interfaces not necessarily the private ones. Also you can cast the return value to a List easily (not something you can easily do when passing List as argument of type ArrayList). Please enter a bug if you think we should fix. Thanks. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-19 15:28 Message: Logged In: YES user_id=1926652 Originator: NO Was it your intention to have getChildren returning ArrayList<> and not List<>? public ArrayList<String> getChildren(String path, Stat stat, Watcher watcher) throws KeeperException.NoNodeException ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-19 12:30 Message: Logged In: YES user_id=1926680 Originator: NO committed revision 168. Thanks Pat. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-16 14:41 Message: Logged In: YES user_id=12853 Originator: YES Here's an updated patch. File Added: parameterizedlist2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-15 14:33 Message: Logged In: YES user_id=1926680 Originator: NO gievn the updates to zookeeper this patch does not apply cleanly. Pat can you upload a new patch for this that applied to the trunk? ---------------------------------------------------------------------- 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-19 23:40:51
|
Patches item #1944401, was opened at 2008-04-16 21:43 Message generated for change (Comment added) made by mahadevkonar 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: Closed >Resolution: Fixed 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: Mahadev Konar (mahadevkonar) Date: 2008-05-19 23:40 Message: Logged In: YES user_id=1926680 Originator: NO this has already been fixed as part of the https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944441&group_id=209147 Closing this bug. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-19 21:33 Message: Logged In: YES user_id=12853 Originator: YES different patch, similar underlying issue. This patch should also be applied. It changes the callbacks intf rather than the zk client interface. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-19 19:34 Message: Logged In: YES user_id=1926680 Originator: NO Pat, is this part of the ArrayList -> List change filed via https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944441&group_id=209147 ? ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 21: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 21:36 Message: Logged In: YES user_id=154690 Originator: NO +1 patch looks good ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-16 22: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-19 23:33:35
|
Patches item #1944401, was opened at 2008-04-16 14:43 Message generated for change (Comment added) made by phunt 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: Patrick Hunt (phunt) Date: 2008-05-19 14:33 Message: Logged In: YES user_id=12853 Originator: YES different patch, similar underlying issue. This patch should also be applied. It changes the callbacks intf rather than the zk client interface. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-19 12:34 Message: Logged In: YES user_id=1926680 Originator: NO Pat, is this part of the ArrayList -> List change filed via https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944441&group_id=209147 ? ---------------------------------------------------------------------- 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-19 23:07:18
|
Patches item #1944441, was opened at 2008-04-16 16:01 Message generated for change (Comment added) made by phunt 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: Fixed 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: Patrick Hunt (phunt) Date: 2008-05-19 16:07 Message: Logged In: YES user_id=12853 Originator: YES You mean DataTree.java right? I think it's ok in this case. The method is private to zk implementation -- I updated the public interfaces not necessarily the private ones. Also you can cast the return value to a List easily (not something you can easily do when passing List as argument of type ArrayList). Please enter a bug if you think we should fix. Thanks. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-19 15:28 Message: Logged In: YES user_id=1926652 Originator: NO Was it your intention to have getChildren returning ArrayList<> and not List<>? public ArrayList<String> getChildren(String path, Stat stat, Watcher watcher) throws KeeperException.NoNodeException ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-19 12:30 Message: Logged In: YES user_id=1926680 Originator: NO committed revision 168. Thanks Pat. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-16 14:41 Message: Logged In: YES user_id=12853 Originator: YES Here's an updated patch. File Added: parameterizedlist2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-15 14:33 Message: Logged In: YES user_id=1926680 Originator: NO gievn the updates to zookeeper this patch does not apply cleanly. Pat can you upload a new patch for this that applied to the trunk? ---------------------------------------------------------------------- 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-19 22:28:00
|
Patches item #1944441, was opened at 2008-04-16 16:01 Message generated for change (Comment added) made by akornev 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: Closed Resolution: Fixed 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: Andrew Kornev (akornev) Date: 2008-05-19 15:28 Message: Logged In: YES user_id=1926652 Originator: NO Was it your intention to have getChildren returning ArrayList<> and not List<>? public ArrayList<String> getChildren(String path, Stat stat, Watcher watcher) throws KeeperException.NoNodeException ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-19 12:30 Message: Logged In: YES user_id=1926680 Originator: NO committed revision 168. Thanks Pat. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-16 14:41 Message: Logged In: YES user_id=12853 Originator: YES Here's an updated patch. File Added: parameterizedlist2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-15 14:33 Message: Logged In: YES user_id=1926680 Originator: NO gievn the updates to zookeeper this patch does not apply cleanly. Pat can you upload a new patch for this that applied to the trunk? ---------------------------------------------------------------------- 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-19 22:05:13
|
Bugs item #1967467, was opened at 2008-05-19 15:05 Message generated for change (Tracker Item Submitted) made by Item Submitter You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1967467&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: c client Group: 3.0.0 Status: Open Resolution: None Priority: 5 Private: No Submitted By: Patrick Hunt (phunt) Assigned to: Andrew Kornev (akornev) Summary: zookeeper_init doc needs clarification Initial Comment: the zookeeper_init document needs to be clarified on two points: 1) that when it returns the client may/maynot have an active session on the server. init call is asyncronous - notification comes back through the watcher when the session is established. 2) this section seems to be wrong. If a client id is invalid (say you pass clientid 1 on a new zk instance) a new session is not created, rather the client's zk handle (when later used) complains that the session is expired. * \param clientid the id of a previously established session that this * client will be reconnecting to. Pass 0 if not reconnecting to a previous * session. If the session timed out or the id is invalid, a new * session will be automatically generated. Clients should check the actual * session id by calling \ref zoo_client_id. Notice that the server message (console) is also misleading - it seems to indicate that the session was reestablished, but in reality is has not. A more clear log message on the server would help here. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1967467&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-19 19:34:45
|
Patches item #1944401, was opened at 2008-04-16 21:43 Message generated for change (Comment added) made by mahadevkonar 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: Mahadev Konar (mahadevkonar) Date: 2008-05-19 19:34 Message: Logged In: YES user_id=1926680 Originator: NO Pat, is this part of the ArrayList -> List change filed via https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1944441&group_id=209147 ? ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 21: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 21:36 Message: Logged In: YES user_id=154690 Originator: NO +1 patch looks good ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-16 22: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-19 19:30:12
|
Patches item #1944441, was opened at 2008-04-16 23:01 Message generated for change (Comment added) made by mahadevkonar 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: Closed >Resolution: Fixed 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: Mahadev Konar (mahadevkonar) Date: 2008-05-19 19:30 Message: Logged In: YES user_id=1926680 Originator: NO committed revision 168. Thanks Pat. ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-16 21:41 Message: Logged In: YES user_id=12853 Originator: YES Here's an updated patch. File Added: parameterizedlist2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-15 21:33 Message: Logged In: YES user_id=1926680 Originator: NO gievn the updates to zookeeper this patch does not apply cleanly. Pat can you upload a new patch for this that applied to the trunk? ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 21: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 21:30 Message: Logged In: YES user_id=154690 Originator: NO +1 patch looks good ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-17 16: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-16 21:41:41
|
Patches item #1944441, was opened at 2008-04-16 16:01 Message generated for change (Comment added) made by phunt 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: Patrick Hunt (phunt) Date: 2008-05-16 14:41 Message: Logged In: YES user_id=12853 Originator: YES Here's an updated patch. File Added: parameterizedlist2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-15 14:33 Message: Logged In: YES user_id=1926680 Originator: NO gievn the updates to zookeeper this patch does not apply cleanly. Pat can you upload a new patch for this that applied to the trunk? ---------------------------------------------------------------------- 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-16 17:44:15
|
Patches item #1963584, was opened at 2008-05-13 22:56 Message generated for change (Comment added) made by phunt You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1963584&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: Benjamin Reed (breed) Summary: fix build.xml cobertura test harness Initial Comment: Need to fail if the junit tests fail - missing fail is causing build to be successful even if cobertura fails. (ci is running cobertura and not std test target) ---------------------------------------------------------------------- >Comment By: Patrick Hunt (phunt) Date: 2008-05-16 10:44 Message: Logged In: YES user_id=12853 Originator: YES Ben please merge this asap to address ci issue. Thanks. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1963584&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-15 22:07:13
|
Bugs item #1958195, was opened at 2008-05-05 19:37 Message generated for change (Comment added) made by mahadevkonar You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1958195&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: Erik Hetzner (egh) Assigned to: Mahadev Konar (mahadevkonar) Summary: log4j is not present in zookeeper.jar Initial Comment: The attached patch includes log4j classes in the zookeeper.jar file. ---------------------------------------------------------------------- >Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-15 22:07 Message: Logged In: YES user_id=1926680 Originator: NO fixed in trunk. ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 19:51 Message: Logged In: YES user_id=1926680 Originator: NO here is another version of the patch that will allow scripts to change the log4 settings. File Added: log4j_build_2.patch ---------------------------------------------------------------------- Comment By: Mahadev Konar (mahadevkonar) Date: 2008-05-06 19:05 Message: Logged In: YES user_id=1926680 Originator: NO hi erik, thanks for your patch. For now we will not be including log4j in zookeeper.jar. I am uploading a patch that builds log4j.properties in zookeeper.jar. So all we need in the classpath is log4.jar and zookeeper.jar. Does that seem good enough? File Added: log4j_build_1.patch ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 22:52 Message: Logged In: YES user_id=12853 Originator: NO Assigned to Mahadev who is working on log4j/zk.jar changes (Ben is looking at the startup script in a separate patch 1951806 ) ---------------------------------------------------------------------- Comment By: Patrick Hunt (phunt) Date: 2008-05-05 22:42 Message: Logged In: YES user_id=12853 Originator: NO Thanks egh -- I further updated the page to reference the zookeeper logging page. The packaging issues are being worked on and should be addressed shortly. Regards. ---------------------------------------------------------------------- Comment By: Erik Hetzner (egh) Date: 2008-05-05 22:15 Message: Logged In: YES user_id=67250 Originator: YES Ok, I modified the wiki tutorial to remove information stating that the .jar file can be used as a standalone jar. You might also want to remove the .jar file distributed on sf.net, because it is impossible to run this file using -jar. ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-05 21:20 Message: Logged In: YES user_id=154690 Originator: NO It is intentional. The fat jar proponent was out voted. We will be doing a new release shortly that distributes the java component as a tar.gz with scripts to start things up. ---------------------------------------------------------------------- Comment By: Erik Hetzner (egh) Date: 2008-05-05 19:44 Message: Logged In: YES user_id=67250 Originator: YES Perhaps this is intentional? It means that the binary server jar distributed via sf.net is not stand alone, however. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1958195&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-15 21:33:24
|
Patches item #1944441, was opened at 2008-04-16 23:01 Message generated for change (Comment added) made by mahadevkonar 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: Mahadev Konar (mahadevkonar) Date: 2008-05-15 21:33 Message: Logged In: YES user_id=1926680 Originator: NO gievn the updates to zookeeper this patch does not apply cleanly. Pat can you upload a new patch for this that applied to the trunk? ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-01 21: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 21:30 Message: Logged In: YES user_id=154690 Originator: NO +1 patch looks good ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-04-17 16: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-14 05:56:06
|
Patches item #1963584, was opened at 2008-05-13 22:56 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=1963584&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: Benjamin Reed (breed) Summary: fix build.xml cobertura test harness Initial Comment: Need to fail if the junit tests fail - missing fail is causing build to be successful even if cobertura fails. (ci is running cobertura and not std test target) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1963584&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-13 14:55:51
|
Bugs item #1963141, was opened at 2008-05-13 07:55 Message generated for change (Tracker Item Submitted) made by Item Submitter You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1963141&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: Benjamin Reed (breed) Assigned to: Nobody/Anonymous (nobody) Summary: Need to do path validation Initial Comment: ZooKeeper needs to do better path validation at the client and server. We need to check for null paths as well as make sure the paths conform to the restrictions in http://zookeeper.wiki.sourceforge.net/ZooKeeperDataModel ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008544&aid=1963141&group_id=209147 |
|
From: SourceForge.net <no...@so...> - 2008-05-12 23:04:19
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Comment added) 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: Closed Resolution: Fixed 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-12 16:04 Message: Logged In: YES user_id=154690 Originator: NO Fixes bug: [ 1948141 ] KeeperException should be sub-classed ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-12 16:02 Message: Logged In: YES user_id=154690 Originator: NO Committed revision 165. Thanx Ted! ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 15:41 Message: Logged In: YES user_id=972206 Originator: YES Here is a slight fix. File Added: 1948141-d.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-12 15:28 Message: Logged In: YES user_id=154690 Originator: NO I think you might be using an old trunk. I'm getting patch failures. There are also a couple of changes you are still reverting in ClientCnxn: you shouldn't be reverting the final decls. In ZooKeeper: you are reverting the watcher fix -- the watcher needs to be set before the ClientCnxn is constructed. As soon as I get a working patch I'll apply it. (I want this one to be the next one to go in.) Thanx Ted! ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 14:52 Message: Logged In: YES user_id=972206 Originator: YES Here is a revised patch that goes against trunk. It should be applied soon to avoid another set of conflicts. File Added: 1948141-d.patch ---------------------------------------------------------------------- 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 23:03:49
|
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: Closed >Resolution: Fixed 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: Benjamin Reed (breed) Date: 2008-05-12 16:03 Message: Logged In: YES user_id=154690 Originator: NO Fixed in patch: [ 1949196 ] KeeperException should be sub-typed ---------------------------------------------------------------------- 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 23:03:29
|
Bugs item #1948141, was opened at 2008-04-21 11:02 Message generated for change (Comment added) 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: Benjamin Reed (breed) Date: 2008-05-12 16:03 Message: Logged In: YES user_id=154690 Originator: NO Fixed in patch: [ 1949196 ] KeeperException should be sub-typed ---------------------------------------------------------------------- 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 23:02:10
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Comment added) 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: Closed >Resolution: Fixed 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-12 16:02 Message: Logged In: YES user_id=154690 Originator: NO Committed revision 165. Thanx Ted! ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 15:41 Message: Logged In: YES user_id=972206 Originator: YES Here is a slight fix. File Added: 1948141-d.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-12 15:28 Message: Logged In: YES user_id=154690 Originator: NO I think you might be using an old trunk. I'm getting patch failures. There are also a couple of changes you are still reverting in ClientCnxn: you shouldn't be reverting the final decls. In ZooKeeper: you are reverting the watcher fix -- the watcher needs to be set before the ClientCnxn is constructed. As soon as I get a working patch I'll apply it. (I want this one to be the next one to go in.) Thanx Ted! ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 14:52 Message: Logged In: YES user_id=972206 Originator: YES Here is a revised patch that goes against trunk. It should be applied soon to avoid another set of conflicts. File Added: 1948141-d.patch ---------------------------------------------------------------------- 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 22:41:35
|
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 15:41 Message: Logged In: YES user_id=972206 Originator: YES Here is a slight fix. File Added: 1948141-d.patch ---------------------------------------------------------------------- Comment By: Benjamin Reed (breed) Date: 2008-05-12 15:28 Message: Logged In: YES user_id=154690 Originator: NO I think you might be using an old trunk. I'm getting patch failures. There are also a couple of changes you are still reverting in ClientCnxn: you shouldn't be reverting the final decls. In ZooKeeper: you are reverting the watcher fix -- the watcher needs to be set before the ClientCnxn is constructed. As soon as I get a working patch I'll apply it. (I want this one to be the next one to go in.) Thanx Ted! ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 14:52 Message: Logged In: YES user_id=972206 Originator: YES Here is a revised patch that goes against trunk. It should be applied soon to avoid another set of conflicts. File Added: 1948141-d.patch ---------------------------------------------------------------------- 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 22:28:31
|
Patches item #1949196, was opened at 2008-04-22 15:07 Message generated for change (Comment added) 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-12 15:28 Message: Logged In: YES user_id=154690 Originator: NO I think you might be using an old trunk. I'm getting patch failures. There are also a couple of changes you are still reverting in ClientCnxn: you shouldn't be reverting the final decls. In ZooKeeper: you are reverting the watcher fix -- the watcher needs to be set before the ClientCnxn is constructed. As soon as I get a working patch I'll apply it. (I want this one to be the next one to go in.) Thanx Ted! ---------------------------------------------------------------------- Comment By: Ted Dunning (tedunning) Date: 2008-05-12 14:52 Message: Logged In: YES user_id=972206 Originator: YES Here is a revised patch that goes against trunk. It should be applied soon to avoid another set of conflicts. File Added: 1948141-d.patch ---------------------------------------------------------------------- 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 22:24:30
|
Patches item #1961779, was opened at 2008-05-10 23:17 Message generated for change (Comment added) made by akornev 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: Closed >Resolution: Accepted 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. ---------------------------------------------------------------------- >Comment By: Andrew Kornev (akornev) Date: 2008-05-12 15:24 Message: Logged In: YES user_id=1926652 Originator: NO Committed at revision 164 ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-12 15:23 Message: Logged In: YES user_id=1926652 Originator: NO +1 patch looks good ---------------------------------------------------------------------- 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 22:23:17
|
Patches item #1961779, was opened at 2008-05-10 23:17 Message generated for change (Comment added) made by akornev 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. ---------------------------------------------------------------------- Comment By: Andrew Kornev (akornev) Date: 2008-05-12 15:23 Message: Logged In: YES user_id=1926652 Originator: NO +1 patch looks good ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1008546&aid=1961779&group_id=209147 |
|
From: Benjamin R. <br...@ya...> - 2008-05-12 22:08:59
|
I have grouped the bugs, feature requests, and patches into 3.0.0 and made some initial assignments. Mahadev and Andrew bore the brunt of the assignments, please feel free to trade and decline. Also, if any of the rest of the world has a burning desired to fix something, feel free to volunteer. Flavio, I didn't talk to you about taking on something, so I didn't assign you anything, but I'm sure anyone would love to share with you. We'd like to get an initial 3.0.0 release out soon. (It would be great to go into Apache with it.) So, we need to make some quick progress. I don't expect to have everything done by next Monday, but it would be good to at least have a handle to the size of the task and be asking for help if needed. (I'm sure Andrew will have everything done, but the rest of us are mere mortals) thanx ben |
|
From: SourceForge.net <no...@so...> - 2008-05-12 21:51:58
|
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:52 Message: Logged In: YES user_id=972206 Originator: YES Here is a revised patch that goes against trunk. It should be applied soon to avoid another set of conflicts. File Added: 1948141-d.patch ---------------------------------------------------------------------- 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:37:00
|
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: Closed >Resolution: Accepted 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:37 Message: Logged In: YES user_id=1926652 Originator: NO Committed revision 161 ---------------------------------------------------------------------- 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 |
