[jira] Commented: (ZOOKEEPER-38) headers (version+) in log/snap files
[ https://issues.apache.org/jira/browse/ZOOKEEPER-38?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12636329#action_12636329 ] Hudson commented on ZOOKEEPER-38: - Integrated in ZooKeeper-trunk #101 (See [http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/101/]) headers (version+) in log/snap files Key: ZOOKEEPER-38 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-38 Project: Zookeeper Issue Type: New Feature Components: server Reporter: Patrick Hunt Assignee: Mahadev konar Fix For: 3.0.0 Attachments: ZOOKEEPER-38.patch, ZOOKEEPER-38.patch, ZOOKEEPER-38.patch, ZOOKEEPER-38_1.patch, ZOOKEEPER-38_10.patch, ZOOKEEPER-38_11.patch, ZOOKEEPER-38_2.patch, ZOOKEEPER-38_3.patch, ZOOKEEPER-38_5.patch, ZOOKEEPER-38_6.patch, ZOOKEEPER-38_7.patch, ZOOKEEPER-38_8.patch, ZOOKEEPER-38_9.patch Moved from SourceForge to Apache. http://sourceforge.net/tracker/index.php?func=detailaid=1961767group_id=209147atid=1008547 -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-38) headers (version+) in log/snap files
[ https://issues.apache.org/jira/browse/ZOOKEEPER-38?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12634267#action_12634267 ] Benjamin Reed commented on ZOOKEEPER-38: This looks great Mahadev. Just small comments: - In ZooKeeperServerMain you changed from using the configuration methods to putting everything in the constructor. I think we should stick with the configuration methods. (This same comment applies to Leader and QuorumPeer - In FileSnapLog.deserialize() if the snapshot is bad, you should try an earlier snapshot. (We should add a test case for this) - SnapLog should really be called Snapshot since it does not have a log component - In FileTxnLog.getLogFiles() you should add a comment to the first (fzxid logZxid) check that you are taking advantage of the fact that the files are sorted, so the last time this condition is true will be for the log file that has snapshotZxid. - In FileTxnLog.getLastLoggedZxid() the comment is a bit unclear. I think this method will also have a problem if the last logfile consists of a single corrupt entry. - In FileTxnLog.truncate() you may need to delete files as well if the zxid you are truncing to is in a log file before the last file - TxnLogCompletion is not used and should be removed - The Util class has some methods that are not used that should be removed. headers (version+) in log/snap files Key: ZOOKEEPER-38 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-38 Project: Zookeeper Issue Type: New Feature Components: server Reporter: Patrick Hunt Assignee: Mahadev konar Fix For: 3.0.0 Attachments: ZOOKEEPER-38.patch, ZOOKEEPER-38.patch, ZOOKEEPER-38.patch, ZOOKEEPER-38_1.patch, ZOOKEEPER-38_2.patch, ZOOKEEPER-38_3.patch, ZOOKEEPER-38_5.patch, ZOOKEEPER-38_6.patch, ZOOKEEPER-38_7.patch Moved from SourceForge to Apache. http://sourceforge.net/tracker/index.php?func=detailaid=1961767group_id=209147atid=1008547 -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-38) headers (version+) in log/snap files
[ https://issues.apache.org/jira/browse/ZOOKEEPER-38?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12614540#action_12614540 ] Benjamin Reed commented on ZOOKEEPER-38: I really like how you pulled the code out of the scattered locations around the source tree. I wish you would have pulled them into a couple of classes rather than scattering them into new classes. It seems like it would be really nice to have a Snapshot class and a TxnLog class. I think classes like SerializeUtils, AsyncSnapshotPolicy, Util, FileLogWriter, FileLogProvider, FileDBInfo (perhaps others) could be pulled into these classes. I agree with Mahadev that the extra interfaces seem overkill for the simple requirements of this Jira. The provider classes are also overkill for this Jira. Perhaps in the future we may need something like that, but I'd rather wait for the need than try and foresee a solution now. Mahadev and I will take a crack at collapsing these classes down. headers (version+) in log/snap files Key: ZOOKEEPER-38 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-38 Project: Zookeeper Issue Type: New Feature Components: server Reporter: Patrick Hunt Assignee: Andrew Kornev Fix For: 3.0.0 Attachments: ZOOKEEPER-38.patch, ZOOKEEPER-38.patch, ZOOKEEPER-38.patch Moved from SourceForge to Apache. http://sourceforge.net/tracker/index.php?func=detailaid=1961767group_id=209147atid=1008547 -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-38) headers (version+) in log/snap files
[ https://issues.apache.org/jira/browse/ZOOKEEPER-38?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=1261#action_1261 ] Mahadev konar commented on ZOOKEEPER-38: i have no intention to discuss the basics of object oriented design on jira. sorry if I meant that in anyway. All i was pointing out is that the code has become really hard to read just because we have lots and lots of unnecessary interfaces and there implementations. I am not arguing against interfaces. I understand that interfaces are not just for pluggalbility. I apologize for the lack of my knowledge and my failure to understand the use of interfaces in some of the places! ill let someone else review the code!! Though For the patch submitters knowledge I have read some of the design patterns books. headers (version+) in log/snap files Key: ZOOKEEPER-38 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-38 Project: Zookeeper Issue Type: New Feature Components: server Reporter: Patrick Hunt Assignee: Andrew Kornev Fix For: 3.0.0 Attachments: ZOOKEEPER-38.patch, ZOOKEEPER-38.patch Moved from SourceForge to Apache. http://sourceforge.net/tracker/index.php?func=detailaid=1961767group_id=209147atid=1008547 -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-38) headers (version+) in log/snap files
[ https://issues.apache.org/jira/browse/ZOOKEEPER-38?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12611885#action_12611885 ] Mahadev konar commented on ZOOKEEPER-38: the patch looks good --- here are some of my comments --- also I am not finished reviewing it :). its a long patch --- 0) overall there are too many interfaces in the patch. though i uinderstand that interfaces make things pluggable but I think we should restrict only wehn we need it and create interfaces only when we seriously think that it would be good to have it pluggable. 1) DataTereeBuilder is an interface and has only one subclass --- i think that we should not make this pluggable and just have a datatree class instead of the interfaces and implementing classes. 2) FileDBINFO implements DBINFO but FILEDBINFO Is the only subclass of DBINFO. I think here again we should just keep it simple and not make this an interface and pluggable. 3) Snapshotting plicy is again pluggable -- we should also not make this pluggable and let it work our default implementation. 4) we have arguments passing around as url though there are not true url's in java -- but just strings. if we really want to make this pluggable and use url's then we should use URL class in java and use its methods for checking what kind of provider we want it to return. like create a URL(string) and url.getProtocol.equals(file) then we retrun our default one. 5) i dont see any use case of more persistence layers as -D expressions? what if we had something like this -Dpersisten.provider=classname and then just use reflections to create the provider class else we just return the default provider. I think it makes the code simpler. i am not done reviewing the patch entirely. :) headers (version+) in log/snap files Key: ZOOKEEPER-38 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-38 Project: Zookeeper Issue Type: New Feature Components: server Reporter: Patrick Hunt Assignee: Andrew Kornev Fix For: 3.0.0 Attachments: ZOOKEEPER-38.patch, ZOOKEEPER-38.patch Moved from SourceForge to Apache. http://sourceforge.net/tracker/index.php?func=detailaid=1961767group_id=209147atid=1008547 -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
