Clay, This looks good. I think it is ready to push.
Please file the bugs/RFEs suggested. Also have you addressed the comments from the code walk through we did of file: publish_manifest.py ? Joe Clay Baenziger wrote: > 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. > OK. >> 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. > OK >> 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. > Thanks for the explanation. >> 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. > Ah, OK. >> 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. > Please file a bug/RFE. >> 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). > I saw this in the 2.6 docs. http://docs.python.org/library/threading.html?highlight=daemon#threading.Thread.setDaemon So as you point out not applicable. >> (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 > OK. Thanks for thinking about it. >> 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... > I used spell(1) ;) >> 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 > OK. >> 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 > Great. >> 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. > Yes. Thank you! >> 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... So we have that in common! ;) > >> 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?) > That the kind of thing I have been thinking of. >> 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. > Please file a bug/RFE. >> 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. > Thanks. >> 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. > Ah, yes. Thanks. >> 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. > Hum. I think deterministic would be good. Perhaps an RFE for evaluation/improvement? >> Issue/Question: >> --------------- >> >> 100 response = name >> >> Why is "response" needed? > > Thank you, it's not > OK. >> 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. > Great. >> 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. > OK. Fair enough. I just wanted to spur the thought. >> 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. > OK. I am sure I was doing something wrong. >> 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 > Thanks >> 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. > OK. >> 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 >>
