Hi Dave,
Thank you very much for taking the time to code review this! My my
please let me know if I'm okay to push as soon as you can. My current
webrev is at:
http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc0/.
Thank you,
Clay
On Thu, 9 Oct 2008, Dave Miner wrote:
> General stylistic issues:
>
> - inconsistent with the existing python code in the project, which uses
> 8 character indent.
PEP 008 says to use 4 spaces, as a recommendation by the Python community
and I see some continuation indents in the distro constructor use 4
spaces, however, most of the code uses tabs instead of spaces. Again the
PEP (and -t/-tt flags to Python) strongly discourage mixing tabs and
spaces. I'd like to defer to the Python community's recommendations here;
but have implemented the standard of tabs and 4 space continuation indents
which the DC and other AI Python code follows.
> - use more vertical whitespace to separate stanzas within functions and
> methods to improve readability
I've tried to add some for readability's sake.
> - much more commenting, especially of algorithms and assumptions. My
> feedback would likely be more extensive if I better understood the
> theory of operation in many of these functions.
After realizing I was misguided to have been told to tone my comments back
I went through commenting
> AI_database.py
>
> 95: "needs"
Thank you
> 104, 125: I don't understand the perceived advantage in combining both
> set and get in a single method, which is generally undesirable. This
> would be much clearer as separate methods, which is basically how it's
> written, anyway.
The difference between response and finished are for if success and we
have a response, or an error condition. They could probably be condensed
with a better more wholistic approach. I don't see the advantage to
breaking them out to be a set_response and get_response method though. It
seems to be a more traditional way of coding it up, versus a way using
Python's interesting control flow abilities. (Jack persuaded me that using
set and get methods would be advantageous for being able to init things
like _ans in the class initializer.) I think this can be done in an RFE
as it's a maintainability enhancement as I perceive it.
> 137: is there a need to use timeouts to prevent the service from being
> unresponsive?
If we don't get a database response it won't be gracefully handled and
CherryPy will spawn a new thread per request (or publish_manifest will get
killed by the user). Though, to prevent stuck processes just hanging out
forever, I did change this to 15 seconds.
> 166: please put errors on stderr
Ah yes; I would agree, but CherryPy doesn't seem to pass STDERR. I should
be using a more robust logging interface, but for the prototype there
simply isn't time. I've taken that out of the comment, however, as this is
a bigger problem. The print function shouldn't be used anywhere where it
shouldn't be replaced with a logging subsystem.
> 170: a comment as to why IMMEDIATE would be useful
I added:
# use SQLite IMMEDIATE isolation to prevent any writers changing
# the DB while we are working on it (but don't use EXCLUSIVE since
# there may be persistent readers)
> 184,190: there's no error information we can pass on from the execute()
> that would help diagnose the problem?
Hi Dave, I'm not sure but some exceptions raised will pass an error string
in. I've modified the function to pass that error string in case it's of
use.
> 232ff: please add whitespace between comment delimiter and comment
Thank you
> 238-end: lots of inconsistent (and excessive) continuation indents to
> clean up. this applies to other files and I won't repeat for them.
I've been trying to indent to the last parenthesis of the block above,
but now follow pretty strictly using a 4 space indent per Jack's
explanation of the standard.
> 272,312: a comment here seems necessary
I have commented the use of the SQL HEX() function in
getSpecificCriteria() and thoroughly commented getCriteria()
> 353: a boolean flag would be preferable to an indirect test on length of
> a string
I could test off getCriteria.next() != None instead of the string length.
It's equivalent to my intention of if queryStr != "SELECT " (as it was set
on 346). I've put in the test against getCriteria for now.
> 371: you're not providing the default here, that would be at a higher
> level, whose behavior shouldn't be assumed here
Thank you for catching the layering error. I've changed the comment
appropriately.
> webserver.py
>
> general comment here is that some versioning of the protocol should be
> considered, perhaps something like the IPS scheme
I think that's a good RFE that should be filed and done before this hits
1.0
> 174ff: excessively indented continuation here.
Yes, I was trying to align with the abspath, but shouldn't have for the
serve_files options.
> 177,187: perhaps we should define a specific x- mime type for our usage?
The download is useful for admins testing with a web browser so that it
doesn't try to interpret it. I don't know if they would try downloading
the file were it a different type? Plus we intend to download this file
with the A/I client so it seems to make sense. Perhaps, I'm a bit confused
what advantages a specific MIME type could buy us though?
> 192: so what are we attempting to return here?
This is the XML-ish criteria list for the AI client to know what's
requested of it. However, perhaps splitting this up wouldn't be a bad
idea. Also, this shouldn't be testing against the postData string, but
using cherrypy handlers for POST versus GET (as per
http://www.cherrypy.org/wiki/RequestObject).
I've added a comment for now
> 246: seems like a questionable redirection to me (218 seems a little
> less odd, though still somewhat questionable). A 404 seems more
> appropriate for what looks like a malformed request.
I agree for 246. I've changed not to a 404, but a 403 with the message
"Index listing denied".
> 269,271: these should be configurable
I have made these command line options
> Comments below are against revised version from
> http://cr.opensolaris.org/~clayb/AI_server/webrev1/
>
> publish_manifest.py
>
> Will this support publishing via non-CLI interfaces as presently structured?
A U/I can import publish_manifest and the much of the work in
parseOptions() can be done by the U/I, but the main logic of publishing
the manifest would be in publish_manifest not the U/I.
> 1: missing CDDL
CDDL'ized
> 132,143,159,169: so what Python would that be? Is it one we actually
> need to run on?
The thought was that A/I could be independent of Solaris at some point
(i.e. run on Linux/OS X or Windows). It seems IEEE754 support in Python
is a bit unstable and I preferred to play it safe for now.
> 135: Assuming we do actually need this, I don't really see the point in
> a separate upper-bound value for MAC vs. non-MAC. It's just a big number.
Good point I don't either.
> 156ff: need much more commenting here about what's happening
Yes, checkCriteria() is a scary, scary function. I have broken it up to
make it much more approachable.
> 214: "determine"
Thank you, I'll remember to aspell(1) more often
> 303: not obvious why they must be the same; if so, why would we bother
> copying in the new one?
We don't have a way to support having two manifests with the same name, so
we warn the user that there's a difference but that the criteria was still
added to the database already. There was a bug, we shouldn't copy the
new manifest in if it's already (same or not).
> 307ff: how about doing this join() once and not 5 times?
Yes, this does clean up the code nicely.
> 401: this comment doesn't appear to match the code
My intention for the comment matches the code but it's unclear. I change
the comment and simplified it
> 440: comment here, please
I changed tag to a more descriptive name and added comments
> 539: "separate"
Yes late night my spelling goes down hill
> 561: "validation"
A lot...
> verifyXML.py
>
> 1: missing CDDL
CDDL'ized
> 38: "receive"
Zzz... Zzz... Type... Type... Zzz... Zzz.. Thank you again for the idea to
run aspell against my code
> Dave
>
