[jira] [Commented] (GIRAPH-270) Improve TestManualCheckpoint

["Alessandro Presta (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GIRAPH-270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13431214#comment-13431214 ] 

Alessandro Presta commented on GIRAPH-270:
------------------------------------------

Good start!

TestCheckpoints looks clean, but what about a comment explaining what it tests (I see an empty javadoc)? E.g. "Tests correctness of reloading from a checkpoint." Also, maybe you can rename it to something more explicit like TestLoadCheckpoint.

What looks fragile is how SimpleIncrementVertex depends on the particular graph which is defined in TestCheckpoints.
A way to fix this would be to make the vertex a static inner class of the test case, to make the dependency more evident.

Regarding the vertex itself:
- the checks are a bit obscure, and I don't like the call to System.exit(). I would break them down so that for each one you can explain what it checks (e.g. "checks the vertex value") and throw an informative exception. If you make it an inner class as I suggested, you can also use assertions from JUnit which are way cleaner.
- the javadoc here is a little confusing, I would go for something more concise. Better to explain the algorithm and the various checks as inline comments than all at once.
- maybe you should also check that the number of edges and messages is 1?

Are you still going to fix TestManualCheckpoint? Since that relies on aggregators, we can block this on GIRAPH-259 which hopefully will be committed in a matter of days. You can add the fix to this patch (it will require a little rebase when GIRAPH-259 goes in).

                
> Improve TestManualCheckpoint
> ----------------------------
>
>                 Key: GIRAPH-270
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-270
>             Project: Giraph
>          Issue Type: Bug
>            Reporter: Alessandro Presta
>            Assignee: Aravind Anbudurai
>         Attachments: Improve_TestManualCheckpoint.patch
>
>
> A good test case for checkpointing is important as we experiment, for example, with storing messages separately to enable out-of-core messaging.
> There are two problems with TestManualCheckpoint:
> - It's only checking that the sum of vertex ids is invariant. This doesn't guarantee that we're correctly restoring vertex values, edges, and messages.
> - The "restarted job" seems to actually start from scratch, and overwrite the previous job's checkpoints. What we are expecting, instead, is the restarted job to load the latest checkpoint and start from there.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira