On 01/11/10 07:45 PM, Sarah Jelinek wrote: > Hi Joe, > > Thank you for reviewing this! Comments inline.. ... >> Question/Issue: >> --------------- >> >> p.6 >> >> 2.3 Logging and Error Reporting >> >> Should this be: "Logging, Error Reporting and Progress reporting" ? > > No, I don't think so. The engine itself manages the progress reporting > and each checkpoint is responsible for its own implementation of > progress reporting. There is no 'Progress Reporting' class. >
Right. The report_progress() method in 2.1 is how a checkpoint can report intermediate progress; otherwise, progress is updated at checkpoint boundaries. > >> Question/Issue: >> --------------- >> >> p.6 >> >> 2.3 Logging and Error Reporting >> >> open(), log_message(), close() >> >> I think better names might make the code more readable by indicating >> these are logger APIs. > > >> e.g. >> >> log_open(), log_message(), log_close() >> > > Yes, I agree. I will change. > Actually, I disagree; the log_* nomenclature is excessively verbose in OO-style invocations, where you're invoking a method on an object. The true mistake I made is the log_message() method. For example, logger.open() logger.print() logger.close() should be concise and clear, while: logger.log_open() logger.log_message() logger.log_close() seems redundant. ... >> Suggestion: >> ----------- >> >> I think a checkpoint should implement it's own clean up. Each checkpoint >> should know what and how to cleanup after itself. >> >> I think the Execution Classes method: cleanup_checkpoints() should invoke >> the checkpoints cleanup entry point. >> >> Maybe the Execution Class could contain a register_cleanup_checkpoint() >> method. > > I do think each checkpoint should cleanup after itself, and that is the > expectation. The cleanup_checkpoints() method is something called by the > InstallerApp on the ExecutionEngine object. I am not sure in which > scenarios this has to be called, I suppose it could be called in the > event of an unexpected checkpoint failure or something like that. In > general, the checkpoint should automatically clean up after itself. > > However, it might be useful to have the checkpoints have a method they > export that the engine calls for cleanup() even if the checkpoint failed > unexpectedly, the engine could still call this method on the checkpoint > and hope it works. > I don't believe having checkpoints clean up after themselves is necessary or desirable. The engine manages whatever state is kept per checkpoint (such as ZFS snapshots, the primary mechanism envisioned here). The checkpoints should know nothing about the mechanisms the engine uses for this. There are two use cases I had in mind here: 1. Deleting all intermediate checkpoints after completion of a successful installation (in other words, delete all the snapshots) 2. Rolling back to the beginning to restart. I don't think either of these require awareness of cleanup on the part of the checkpoints. Dave
