Hi Alok,
Alok Aggarwal wrote:
Hi Dermot,
Thanks for taking a second look.
On Wed, 1 Dec 2010, Dermot McCluskey wrote:
Alok,
Apologies as I haven't had time to properly re-review this.
However, looking briefly through the file
execution_checkpoint.py
the way the Kwargs class is written and used seems a bit unusual.
Kwargs looks like a typical DataObject sub-class, with to_xml and
from_xml
methods but they are not defined as @classmethod functions and
Kwargs is not a sub-class of DataObject.
Having Kwargs be a sub-class of DataObject is actually
ideal and was the way I had it implemented in one of
the earlier versions.
The problem that that poses is then the Kwargs DataObject
ends up in the DOC. This leads to the install_engine
not being able to validate those kwargs since they are a
DataObject and no longer a dictionary as required by the
engine's register_checkpoint() API.
I'm not sure I follow this completely, so please correct me if I've
got this wrong:
Would it not be possible to have Kwargs as a regular DataObject
sub-class, but also have an extra method in this class, called eg to_dict()
which converts the current object to a dictionary - which does basically
what I think lines 227-234 currently do. Then, when you are calling
register_checkpoint(), instead of passing a Kwargs object for the kwargs
keyword param, you would pass in the dictionary returned by calling
Kwargs.to_dict()?
Or even more transparently, kwargs could be a Python property of the
Checkpoint class, which fetches its child(ren) of type Kwargs from the
DOC and calls the to_dict() method on the child(ren)? That way, the
call to the engine would still look something like:
register_checkpoint(... kwargs=checkpoint.kwargs ...)
If the current code you have works, then I suppose it's fine. My main
problem with it is that it might be difficult to maintain.
There's a comment on lines 228-230 in execution_checkpoint.py
to this effect. Perhaps it could be improved? Would you have
suggestions for improvement?
So, I am assuming that you intend to handle all the sub-elements of
Checkpoint within the Checkpoint class itself and by calling the Kwargs
class methods directly, rather than relying on the DOC to take care
of this for you?
And the fact that some of the methods of Kwargs are called to_xml and
from_xml is just a co-incidence? If so, then I suggest changing those
method names as they might be confusing.
Yep, I agree I'll change the names to make it clearer.
However, I would also have expected that you would be setting the
generates_xml_for_children
flag somewhere in the Checkpoint class, so that the DOC knows
not to try to process the sub-elements of Checkpoint itself, as it will
otherwise do.
So, I would suggest that at least you rename Kwargs.to_xml and
from_xml and
set self.generates_xml_for_children = True in Checkpoint.from_xml().
I can certainly set generates_xml_for_children, is that a public
attribute though?
Yes.
In my testing I don't see the DOC trying to generate xml for the
children, it may just be due to the fact that there aren't any
classes defined that can handle <kwargs>.
That makes sense. But it's definitely more efficient to tell the DOC
that it doesn't need to be checking for registered classes to handle the
children.
Thanks,
- Dermot
Thanks,
Alok
However, I still think it would be easier to have Kwargs (and even some
of its sub-elements) as normal DataObject sub-classes and let the DOC
take
care of connecting it to the Checkpoint objects. I think this will
make the
code much more straight-forward.
- Dermot
On 11/29/10 22:16, Alok Aggarwal wrote:
Just a quick reminder to everyone intending to do
a second pass through, the deadline for providing
review comments is COB, December 1st.
If you intend on reviewing and need more time, please
let me know as well.
Alok
On Thu, 18 Nov 2010, Alok Aggarwal wrote:
Thanks to everyone who reviewed the code for the
DC->CUD project the first time around. Here's round two
that incorporates most of the changes brought up.
Incremental webrev against the initial webrev:
http://cr.opensolaris.org/~aalok/cud_dc-diff2/
Complete webrev:
http://cr.opensolaris.org/~aalok/cud_dc-2/
Some of the most significant changes included here
are:
- DC Logging has been overhauled. logger's transfer_log()
was enhanced as part of CR 7000990 and DC now uses that
instead of managing log files, etc by itself.
- Determining the ZFS mountpoints: This code was reworked
to utilize the zfs code in install_target. The mountpoints
are now determined *after* TI has executed instead of trying
to determine mountpoints ahead of time.
- DC Lockfile: This is now a context manager (used via the
"with" statement) and has added additional streamlining
- Unittests: There are now unittests for the primary DC
application. Coverage is ~ 70%
- A SimpleSchemaElement class has been defined for use by
.. well, simple schema elements. This code will eventually
be moved to the DOC code (there's an outstanding bug on this).
- kwargs/args in execution_checkpoint.py is now a separate class
- Sort files are now included in the source
- Manifest Parser tests that were commented out due to the absence
of DC in slim_source have been uncommented.
Please take a look and provide your feedback by COB, December 1.
Thanks,
Drew and Alok
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
