[
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
