Clay Baenziger wrote: > 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 >
I started to re-review, but stopped, as there are significant discrepancies between your responses and what appears in the webrev, so I think there was an error in generating the webrev. Additionally, this needs to be merged against the current gate, proper bug numbers added to the commit comments, and a crying need to run cstyle. Please do all of these things before asking for another review, we won't get to closure without them. Some additional comments on your responses in-line below, and comments at the end on pieces I hadn't seen before. > 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. > Establishing style is a collective decision, so you should work with the others on the team to sort out a recommendation for style going forward. I have no problem using the PEP standards, but I'm more interested in consistency among our own code. I also continue to see a lack of consistency in what you've don here; continuations are all over the map still in AI_database.py, for example. >> - use more vertical whitespace to separate stanzas within functions and >> methods to improve readability > > I've tried to add some for readability's sake. > I'd like much more. Long functions such as findManifest, getCriteria, etc., need to be much more readable than they are. >> - 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. > Bluntly, I don't regard maintainability issues as RFE's. I don't see any "interesting control flow abilities" here; the same thing could be done (equally badly) in any language I've used. Using presence or absence of an argument to completely switch the sense of a method from get to set is bad design anywhere. >> 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. > A comment as to why 15 seconds would be helpful. >> 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. > I need to see an RFE filed for that. Karen can probably help you with logging since she just overhauled this in DC. >> 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. > As noted above, need to get more strict. at line 250: "instance" >> 272,312: a comment here seems necessary > > I have commented the use of the SQL HEX() function in > getSpecificCriteria() and thoroughly commented getCriteria() > I see no comment related to HEX() in the webrev. >> 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. > My comment was against the line: 353 if len(queryStr) > 7: which still remains, now as 361 in the new webrev. >> 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. > I still see the original comment in this webrev. 399,410: the error messages here still are a layering violation >> 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 > It's a bug, actually. Please file it. this file also continues to need more vertical spacing in the larger functions. >> 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? > I think it would help to validate that you aren't directed to a server (perhaps due to admin mis-configuration) that's providing something other than what you're expecting. Any unknown x- type would seemingly provide the same web browser advantages you get from x-download. >> 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 > Again, I don't see this comment in the posted webrev. >> 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 >> So what's the AI.db file? usr/src/cmd/Makefile: 36: Alphabetize lists such as this. This also hasn't been merged against the current version yet. usr/src/cmd/ai-webserver/Makefile: continuations in Makefiles conventionally are indented to align with the item they're continuing, and we don't spread content that fits in a single line across a continuation. Consult existing Makefiles for stylistic examples. Copying existing practice is much preferred over making up your own. 95 et al: 744 is a most unusual mode. 755 for executables, please, though in general we just don't bother with setting modes in these rules and do so on the install phase. default.xml why do we set swap size in the default manifest? Can't we use the algorithms in the orchestrator to compute this automatically in the default case? SUNWinstalladm-tools/prototype_com: I don't think the schema files belong in /var; something in /usr/share, perhaps, seems more appropriate. This would affect Makefile.cmd and Targetdirs. installadm.c: There are so many cstyle errors here I hardly know where to begin, but there are incorrect continuation indents in all of the new code. The rules are simple: 4 space indent on a continuation line, period. Style of switch statements is also incorrect: cases are to be at the same indent level as the switch. Please review a copy of the C style guidelines from http://www.opensolaris.org/os/community/on/, and run cstyle on your code before sending for review. 782,815: incorrect usage of MAXPATHLEN. These aren't path names, so use a constant specific to your usage. Dave
