Activity

The problem is that on the very first iteration of the loop needToSave is set to true and cannot be reset to false after that. It happens because checkpointTime never equals Long.MIN_VALUE while both latestNameCheckpointTime and latestEditsCheckpointTime do.

Konstantin Shvachko
added a comment - 23/Feb/09 22:54 The problem is that on the very first iteration of the loop needToSave is set to true and cannot be reset to false after that. It happens because checkpointTime never equals Long.MIN_VALUE while both latestNameCheckpointTime and latestEditsCheckpointTime do.

I'm afraid that didn't work - at the point loadfsimage is called, the files already exist. I replaced the overly complicated statement that was trying to determine if there were more than one checkpoint times with a set of checkpoint times. It's more elegant - after processing all the storage directories, check the cardinality of the set, if it's greater than one, there were more than one checkpoint times recorded and needToSave should be set to true.

Fixing this bug revealed another bug where the editLog was only being opened when the fsimage was being saved. To fix this, I added a call to open the editlog to the end of loadfsimage.

Also added some minor readability improvements to the relevant sections of code.

Passes all unit tests.

[exec] -1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
[exec] Please justify why no tests are needed for this patch.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

I can't figure out a reasonable way to unit test these fixes, though I've manually tested them several times.

Jakob Homan
added a comment - 08/May/09 18:05 I'm afraid that didn't work - at the point loadfsimage is called, the files already exist. I replaced the overly complicated statement that was trying to determine if there were more than one checkpoint times with a set of checkpoint times. It's more elegant - after processing all the storage directories, check the cardinality of the set, if it's greater than one, there were more than one checkpoint times recorded and needToSave should be set to true.
Fixing this bug revealed another bug where the editLog was only being opened when the fsimage was being saved. To fix this, I added a call to open the editlog to the end of loadfsimage.
Also added some minor readability improvements to the relevant sections of code.
Passes all unit tests.
[exec] -1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] -1 tests included. The patch doesn't appear to include any new or modified tests.
[exec] Please justify why no tests are needed for this patch.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
I can't figure out a reasonable way to unit test these fixes, though I've manually tested them several times.

The "the overly complicated statement" was intended to avoid using extra objects like HashSet. But your approach works too.
Thanks for noting that editLog should be opened explicitly if the image was not saved.
I do not see any rational in changing the order of the imports. They were lexicographically ordered. Could you please restore the original order.
+1 other than that.

Konstantin Shvachko
added a comment - 08/May/09 22:12 The "the overly complicated statement" was intended to avoid using extra objects like HashSet. But your approach works too.
Thanks for noting that editLog should be opened explicitly if the image was not saved.
I do not see any rational in changing the order of the imports. They were lexicographically ordered. Could you please restore the original order.
+1 other than that.