Hi Jean, Thank you for the review. My comments inline..
> Section 2.1: > > "The engine is capable of beginning execution at the next unreached > checkpoint in the sequence" > > I find that confusing. If you haven't reached that checkpoint, how can > you resume execution at it? we aren't really resuming. we are starting that checkpoint at its beginning. > (An aside, probably should change begin to resume) I think begin is better than resume. The idea is that the checkpoints can be run independently(within sequence of course), and that the engine can be told which checkpoint to begin execution at. > Under checkpoint methods: > You might consider methods such as resume, pause, rollback. Better names > could be selected but I suspect you get the idea. I am not sure we need those. I get the idea, but I am not clear why you think we need them. A checkpoint must be run in its entirety, and the engine is responsible for managing what has been run and where it needs to start next. So, the checkpoint itself doesn't manage that. Can you clarify why you think these are required? Could be we haven't been clear enough in our description. > > Section 3.1: > > I foresee making the validate a separate method. I think the syntactic validation happens as part of the parsing but we may need to consider a validate() method. > > Maybe even a store method that calls the DataCollection store_data > method? Not sure on this one. That's interesting. We will need to store the manifest data but I am not sure we need a 'store' method on the ManifestParser class. The class itself will likely call store_data() on the DataCollection when it is done. > > Section 3.2: > Any need to discover zfs datasets too? > Yes, I don't include it in the list, because it is a subclass of filesystem. However, you are the second person who asked this so I will add it to be clear. > The last line in the section references "see Section 3.2". But this is > section 3.2! Fixed. > I'm confused on the fact that it needs a DeviceTarget or LogicalTarget > as a seed. What if you just want it to > discover the system as it does now? Or am I misunderstanding what you > mean here? No we need a seed. Target discovery can be done on different device types. We may not want to start with disks, for example, with Replication, we want to discover zpools, and datasets, not really the physical devices. So, to make Target Discovery general enough we felt that using a seed value would make this class common for use by multiple consumers. > Section 3.2.1.1: > > Partition class: I think you will need more information from the > partition that whether it's active or not and it's associated slices. > Things like type, id etc. Since you're calling out get_active and > set_active I think you might want to call out these as well. You might > want to look at partition.h in lib/libtarget_pymod to get an idea of the > attributes that are used. I agree we will probably need more methods for getting/setting attribute data. I am sure I haven't covered them all. I will take a look at partition.h in the libtarget_pymod for more data. > Slice Class: Similar comment as above. See slice.h instead. > Ok, will do. > Section 3.2.1.2: > > You list the logical base classes in a specific order and then when > giving details on them change the order. Makes it harder to follow. > Also, you don't list dataset class here but you do later when giving > details on each of the classes. Will fix ordering. Dataset is a subclass, but again it isn't clear so I will add this. > Section 3.3: > > "The set of objects required to instantiate a target are same ones that > are were defined for target > discovery, described in Section 3.2" > > Note the "that are were" in this sentence. I think it should be "that > were". Fixed. > What about zfs datasets? > They are considered filesystems, but I will note these also. > Section 3.4: > > Do we need a get and set of the destination? Maybe... let me think on this a bit. I imagine we do. Thanks again for the review. sarah ***** > Jean > > > > > >
