[
https://issues.apache.org/jira/browse/ZOOKEEPER-1835?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13848829#comment-13848829
]
Rakesh R commented on ZOOKEEPER-1835:
-------------------------------------
Thanks for the patch, it looks nice. I have few comments, please see.
# It would be nice to use IOUtils.closeStream(out); instead of following in the
patch
{code}
if (out != null)
out.close();
{code}
# Apache license is missing in the files AtomicFileWritingIdiom.java,
AtomicFileWritingIdiomTest.java
# Please correct the code format. Please see
http://wiki.apache.org/hadoop/ZooKeeper/HowToContribute section for more info.
There are spaces in empty lines and also in java comments like "// and the
caller will be notified (IOException)"
# I couldn't see the following constructor is used anywhere. Is it required?
{code}
public AtomicFileWritingIdiom(File targetFile, WriterStatement wStmt,
String encoding) throws IOException {
this(targetFile, null, wStmt, encoding);
}
{code}
# This is a suggestion only. There are many null checks in the constructor
logic like, if not null do the operation. I think, you have done this to reduce
the code duplication. But I feel, this will create confusions in debugging or
maintainability. Say, by mistake if the passed object is null, this will just
skip the logic and return. Caller will be thinking the operation is
successfully completed. IMHO, its ok to duplicate the constructor logics rather
than null checks and skipping. Whats your opinion?
-Rakesh
> dynamic configuration file renaming fails on Windows
> ----------------------------------------------------
>
> Key: ZOOKEEPER-1835
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1835
> Project: ZooKeeper
> Issue Type: Bug
> Components: quorum
> Affects Versions: 3.5.0
> Environment: Windows 7 64-bit, Oracle Java 1.6.0_32-b05
> Reporter: Bruno Freudensprung
> Attachments: ZOOKEEPER-1835.patch
>
>
> On Windows, reconfig fails to rename the tmp dynamic config file to the real
> dynamic config filename.
> Javadoc of java.io.File.renameTo says the behavior is highly plateform
> dependent, so I guess this should not be a big surprise.
> The problem occurs in
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java that
> could be modified like this:
> + curFile.delete();
> if (!tmpFile.renameTo(curFile)) {
> + configFile.delete();
> if (!tmpFile.renameTo(configFile)) {
> As suggested by Alex in https://issues.apache.org/jira/browse/ZOOKEEPER-1691
> (btw there is more information about my test scenario over there) it is a bit
> "scary" to delete the current configuration file.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
