Karen Tung wrote: > Hi Jean, > > Thank you for making the changes. I still have a few comments: > > - line 34: why combine the 3 separate lines into this one line? I > don't think we need everything from dc_utils.py. Because there is very little in dc_utils.py and I find it annoying to have multiple lines as previously done. I guess just a person "button". I can change if you really want. > - line 409: I think we don't need to mention the empty snapshot here Except that it's kind of important. If you leave the empty snapshot in, the functionality doesn't work correctly. > - line 686: why is the "-r" removed from here as well as the > finalizer_rollback.py? -r is added not removed. The reasoning is that if you wish to rollback to a step and you've deleted later steps this should be legal. Without the -r it doesn't work.
Jean > > Thanks, > > --Karen > > Jean McCormack wrote: >> Karen, >> >> I believe I've addressed all of your concerns. The webrev is at : >> http://cr.opensolaris.org/~jeanm/slim_5170/ >> >> Jean >> >> Karen Tung wrote: >>> Hi Jean, >>> >>> Here are my comments: >>> >>> - Line 402-420. My understanding of this code segment is >>> that you are getting a list of all the snap shots, and making >>> sure that the order of the of checkpoints corresponds to the order >>> of the >>> snapshots list. Your list of checkpoints is built by reading the >>> manifest, >>> so, I understand that the checkpoints will be in the order that's >>> specified. >>> How do we ensure that the output of "zfs list...." will follow the >>> same order? >>> Right now, it just so happens that they follow the same order. >>> Is there a rule or something that zfs datasets/snapshots will be listed >>> in the order they are created? I don't see an option in "zfs list" >>> to sort >>> the datasets by the time they are created or something. That will >>> certainly gurantee the output of "zfs list" will follow the order >>> of things specified in the manifest. I am concern >>> that if "zfs list" ever change the order of the output, then, our >>> code won't work. >>> >>> - lines 346-348: I think it is inefficient to execute a system shell >>> and do a grep on >>> the temp file content since this loop is executed for each item on >>> the step list. I think >>> it is more efficient to read the content of the temp file into a >>> python list and just >>> search the internal list. >>> >>> - line 348: since you already found the step_num here, can we break >>> out the loop? >>> >>> - Lines 333-337 and 389-395 are doing the same thing. I think it is >>> better to have the common >>> code in a function or something. Even better, you can make this >>> function return a >>> list of snapshot names. Also, I just saw something on the python >>> docs page about how >>> to do the shell pipeline in Python. Maybe you want to try something >>> like this? >>> http://docs.python.org/library/subprocess.html#subprocess-replacements >>> >>> Thanks, >>> >>> --Karen >>> >>> Jean McCormack wrote: >>>> Please review the fix for >>>> 5170 DC_determine_Resume_step should check snapshots existence not >>>> .step files. >>>> >>>> Bug: >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=5170 >>>> >>>> Webrev: >>>> http://cr.opensolaris.org/~jeanm/slim_5170/ >>>> >>>> Testing included: >>>> >>>> 1) distro_const build -l <manifest> >>>> >>>> 2) distro_const build -r <legal value> <manifest> >>>> >>>> 3) distro_const build -r <illegal value> <manifest> >>>> >>>> 4) Modify manifest adding another finalizer script. >>>> Repeat 1,2,3 >>>> >>>> 5) Modify manifest removing a finalizer script >>>> Repeat 1,2,3 >>>> >>>> 6) Modify manifest shuffling the finalizer scripts >>>> Repeat 1,2,3 >>>> >>>> Jean >>>> >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >>> >> >
