Hi Joe,
Thank you for this code review. My comments are inline. Please
confirm early today if I've met your requirements, so I may soon push.
My latest webrev is at:
http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc0/.
Thank you,
Clay
On Fri, 10 Oct 2008, Joseph J VLcek wrote:
> Clay
>
> I still have to review bin/publish_manifest.py which I will do over the
> next day or so.
>
> "Please" understand that I am not a Python expert. A lot of my input may
> not seem pertinent simply due to my lack of Python experience. If some
> of my comments seem inapplicable it may simply be due to my lack of
> Python experience. Please forgive that.
>
> Joseph
>
> +-=+-=+-=+-=+-=+-=+-=+-=
>
> General comment:
> ---------------
>
> I think more extensive error checking/trapping with try would help
> us diagnose potential issues.
>
> General comment:
> ----------------
>
> More comments would be valuable. High level description of what
> is doing what and how things interrelate.
>
> Clay, you and I had done a quick TOI over the phone and you had
> sent me an email describing what is what. That kind of stuff should
> be placed in the code as comments.
>
> Thing of the person who will have to diagnose this in 2 or 3 years.
> What would be valuable for them to know and add that as comments.
>
> General question:
> -----------------
>
> How are you testing this code?
Right now the testing is by manual hand testing of the various code paths.
> How are you testing the cherrypy in webserver.py?
> Is there a good way to validate this?
I trust CherryPy to be stable, but it would be evident if it were failing
to serve pages, or generate reasonable errors/redirections, etc.
> bin/AI_database.py - 425 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> Issue/question:
> ----------------
> 31 from threading import Thread
>
> Please confirm is it necessary to "from threading import Thread"
> since you imported threading on the line above.
I believe doing this on two lines is as compact as I can. It allows me to
doing things without having to do threadding.thread.setDaemon... for
example. Otherwise, the initial import threading would be sufficient.
> Issue/question:
> ----------------
> 33 from pysqlite2 import dbapi2 as sqlite
>
> Would it makes sense to consider sqlite3?
>
> http://oss.itsystementwicklung.de/trac/pysqlite/wiki/PysqliteVersions
We are using SQLite3, as that's what PySQLite2 interfaces with. PySQLite
0.x/1.x were for SQLite2.
> Issue:/Question:
> ----------------
>
> 46 self._requests = Queue.Queue()
> Is an infinite queue OK? Or should it have bounds?
This is an excellent RFE to limit queue sizes then for the client
applications to check for queue full exceptions and shutdown assuming the
DB got wedged.
> Issue:
> ------
>
> 48 self._runner.setDaemon(True)
>
> According to docs.python.org setDaemon is: Old API for daemon.
I can't find this same information. I see it looks current on
http://www.python.org/doc/2.5.2/lib/thread-objects.html (and we're only
shipping Python 2.4.4 anyways versus this doc for 2.5.2).
> (JJV: I don't know... I'm learning this on the fly ;)
>
> Issue:
> ------
>
> 56 Ensures resonable DB schema and columns or else raises a
> SystemExit
>
> Misspelling: resonable -> reasonable
Darn missing a's
> Issue:
> ------
>
> 92 self._commitable = commit
> 156 self._commitable = commit
> 169 if self._commitable == True:
>
> Misspelling: commitable -> committable
Yea for private variables
> Issue:
> ------
>
> 95 """ Use commit() to determine if the query need to be
> commited. """
> Misspelling: commited -> committed
Thank you for the help spelling again...
> Issue/Qustion:
> --------------
>
> 102 def response(self, resp = None):
> and
> 125 def finished(self, error = None):
>
> I don't understand the value of having response do both set or retrieve.
>
> Would it be clearer to a future maintainer to separate this
> into: setDBResponse and getDBResponse?
>
> Same comment applies to finished.
>
> Would it be clearer to a future maintainer to separate this
> into: setFinished and isFinished?
To me it seems a good use of Python's exception handling to do this as
one. I'm not sure that it buys much one way or the other
> Issue:
> ------
>
> 108 # Note: _ans not defined set to None so that its
> non-existance is
I'm going to claim I've once known how to spell...
> Misspelling: non-existance -> nonexistence
>
> Issue:/Question:
> ----------------
>
> 131 self._e.set()
>
> The tread is set here on line 131. Where/how is it cleared? Does
> class DBrequest(object): need to provide a mechanism to clear it?
No, once set that DBrequest is fulfilled and will someday be garbage
collected but not used again
> Issue/Question:
> ---------------
>
> 139 self._e.wait()
>
> Shouldn't wait specifiy a timeout so it doesn't hang on stuck
> access and report if the timeout expired?
I've set this to 15 seconds
> Issue/Question:
> ---------------
>
> 241 yield(query.response()[0][0])
> 318 yield str(column)
> 323 else: yield str(column.replace('min',''))
> 341 yield str(colName)
>
> Is your usage of yield correct? It doesn't seem to match my understanding
> from what I read at: www.python.org/doc
I think we cleared up my usage of yield on the phone.
> Issue:
> ------
>
> 246 Returns the manifest names and instane tuple as specified by a
> list of
>
> Misspelling: instane -> insane ;) (or maybe instance ;)
That was me trying to reflect on the code's author and slipping a t in...
> Issue/Question:
> ---------------
>
> 254 queryStr = queryStr[:-5]
>
> What if criteria is empty?
Then one shouldn't be using "findManifestByCriteria". As the criteria
argument is required I'm not sure what I should test for. (That it's a
>0 length, list?)
> queryStr would end being: "SELECT name, instance FROM manifests W"
>
> Perhaps it would be good to validate the arguments passed to all
> of the functions defined in AI_database.py
Due to how quickly this code was written I think an RFE for sanity checks
would be a good idea.
> bin/listManifests.py - 174 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> General comment:
> ---------------
>
> more comments. ;)
>
> Each function definition should have a comment block describing
> functionality, input and output, and errors...
I've added the comment blocks with a bit more information, and I've
commented the code more thoroughly.
> General comment:
> ---------------
>
> Issue/Question:
> ---------------
>
> 87 print headerL1
> 91 print headerL2
> 102 print response + ":"
> 105 print response
>
> i18n ? This code doesn't seem I18N-ed to me.
>
> shouldn't print _() be used?
For strings which can be localized gettext is used i.e.
"len(_("Instance"))" etc, but for the criteria we should use what the
database uses and in particular header/response will contain unpredictable
output depending on the criteria being printed out (from the database) so
not the right place to gettext() to me.
> Also:
>
> Wouldn't it be easy for the alignment of the output
> to easily become skewed.
>
> To ensure output lines up would it be better to explore using
> something like this?:
>
> print '--->%5s<--- --->%5s<--- --->%5s<---' % ("hi", "why", "when")
That might be a good RFE to limit the lengths, right now I hand compute
lengths which works, but may result in very wide outputs; though yours
would give a more deterministic length it would potentially truncate
criteria.
> Issue/Question:
> ---------------
>
> 100 response = name
>
> Why is "response" needed?
Thank you, it's not
> Could this:
> 101 if maxLengths:
> 102 print response + ":"
> 103 printCriteria(DB, maxLengths, name)
> 104 else:
> 105 print response
>
> be changed to:
> 101 if maxLengths:
> 102 print name + ":"
> 103 printCriteria(DB, maxLengths, name)
> 104 else:
> 105 print name
>
> Issue:
> ------
>
> Looks like there is an old, commented out, block of code that should
> be removed.
>
> 107 # AIdb.numManifests(self.AISQL.getQueue())
> 108 #max(len(list(AIdb.getCriteria(self.AISQL.getQueue(),
> 109 # strip = False))), 1)
> 110 #(AIdb.numInstances(manifest, self.AISQL.getQueue()),
> 111 #(AIdb.getCriteria(self.AISQL.getQueue(), strip=False))), 1)
>
Yes, those were turds. My apologies for not grabbing those.
> bin/publish_manifest.py - 601 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
> Not yet fully reviewed
>
> 214 # painful check to determin if this is a range collision
> change determin to: DETErmine
Fixed
> 539 # manifestPath will be unset if we're not using a
> seperate file for
> Change seperate to: separate
Fixed
> 561 print >> sys.stderr, "Error:\tFile %s failed validataion:" % \
> change validataion: to validation
Fixed
> bin/verifyXML.py - 53 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> Issue:
> -----
>
> Missing CDDL header.
Fixed
> Question:
> ---------
>
> The 2 functions defined in verifyXML.py seem so similar it makes
> me wonder if they couldn't easily be combined.
>
> Could
> 10 def verifyDTDManifest(data, xml_dtd):
> and5A
> 35 def verifyRelaxNGManifest(schema_f, data):
> be "easily" combined into:
> def verifyManifest(data, type):
Potentially, except their exceptions and the like are subtly different.
A combined function could be made and receive a flag as to if the XML is
RelaxNG or DTD based I suppose. I'm not sure how much it buys though.
> bin/webserver.py - 273 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> Issue:
> ------
>
> 36 import cherrypy
>
> I think there may be something wrong with cherrypy. In playing with it
> I get a traceback. I am sure this is not your code but I just wanted
> you to beware of it during your testing.
>
> Or maybe I'm just doing something wrong. ?
Without the stack trace I'm not sure.
> Issue/Personal Opinion/Nit:
> ---------------------------
>
> 78 <p>This server has available the following manifests,
>
> I think this may read better as:
>
> This server has the following manifests available,
It now reads as such
> Issue/Question:
> ---------------
>
> What would happen if any of the AIdb. methods failed? Is it possible
> to try/except any possible exception?
They'll handle their own errors, or else fail only for that request of the
webserver. I've tried to keep the webserver from fatally failing since
it's not being run out of SMF at this time.
> var/AI_service_1/ai_schema.rng - 281 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> I did not do an extensive review of this xml.
>
> var/AI_service_1/criteria_schema.rng - 123 lines
> +-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
>
> I did not do an extensive review of this xml.
>
>
>
> Clay Baenziger wrote:
>> Hi all,
>>
>> I've updated the webrev, and sent out a new code tar-ball for folks to
>> work with.
>>
>> This update supports a "universal manifest" (based off the criteria
>> schema) which embeddes the A/I manifest and all SC manifests. This
>> universal manifest will be what is served out by the webserver for parsing
>> by the A/I engine. An example manifest is included as [1] here.
>> Unfortunately, this is a bit cludgy as it's a RelaxNG manifest which
>> includes XML 1.0 DTD based XML (the SMF SC manifests).
>>
>> Also, due to this new universal manifest is an updated criteria schema
>> allowing one to specify the A/I and SMF manifests in the criteria and
>> updates to publish_manifest(1) to correctly verify everything handed in.
>>
>> Lastly, due to some bugs I've found the criteria collision logic got a few
>> tweaks.
>>
>> Tar-ball:
>> http://cr.opensolaris.org/~clayb/AI_server/AI_server.finalCut.1.tgz
>> Webrev (just diffs off previous):
>> http://cr.opensolaris.org/~clayb/AI_server/webrev1/
>>
>> Thank you,
>> Clay
>>
>> [1]:
>> <ai_criteria_manifest>
>> <ai_embedded_manifest>
>> <ai_manifest name="default">
>> <ai_target_device>
>> <target_device_name>c0t0d0</target_device_name>
>> <target_device_type>SCSI</target_device_type>
>>
>> <target_device_vendor>hitachi</target_device_vendor>
>> <target_device_size>20480</target_device_size>
>>
>> <target_device_use_solaris_partition>true</target_device_use_solaris_partition>
>>
>> <target_device_overwrite_root_zfs_pool>true</target_device_overwrite_root_zfs_pool>
>> </ai_target_device>
>> <ai_device_partitioning>
>> <partition_action>create</partition_action>
>> <partition_number>1</partition_number>
>>
>> <partition_start_sector>200</partition_start_sector>
>> <partition_size>20480</partition_size>
>> <partition_type>99</partition_type>
>> </ai_device_partitioning>
>> <ai_device_vtoc_slices>
>> <slice_action>create</slice_action>
>> <slice_number>4</slice_number>
>> <slice_size>20480</slice_size>
>> </ai_device_vtoc_slices>
>> <ai_device_zfs_root_pool>
>>
>> <enable_mirrored_root>true</enable_mirrored_root>
>> <zfs_options>xyz</zfs_options>
>> </ai_device_zfs_root_pool>
>> <ai_device_swap>
>> <ai_swap_size>2048</ai_swap_size>
>> </ai_device_swap>
>> <ai_pkg_repo_default_authority>
>> <main url="http://pkg.opensolaris.org";
>> authname="opensolaris.org"/>
>> <mirror url="" authname=""/>
>> </ai_pkg_repo_default_authority>
>> <ai_packages>
>> </ai_packages>
>> </ai_manifest>
>> </ai_embedded_manifest>
>> <sc_embedded_manifest name = "AI">
>> <!-- <?xml version='1.0'?>
>> <!DOCTYPE service_bundle SYSTEM
>> "/usr/share/lib/xml/dtd/service_bundle.dtd.1">
>> <service_bundle type="profile" name="name">
>> <service name="ai_properties" version="1" type="service">
>> <instance name="default" enabled="true">
>> <property_group name="ai" type="application">
>> <propval name="username" type="astring"
>> value="jack"/>
>> <propval name="password" type="astring"
>> value="jack"/>
>> </property_group>
>> </instance>
>> </service>
>> </service_bundle>
>> -->
>> </sc_embedded_manifest>
>> </ai_criteria_manifest>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
