Hi Clay, Is this rc2 webrev supposed to be the latest and greatest with all of the changes? I am not seeing installadm.c anymore, though that was in an earlier webrev.
Sue Clay Baenziger wrote: > Hi Dave, > I've fixed the webrev, you'll find the newest one (with the head > merged at http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc2. > I've filed the following bugs to address comments not addressed in the > code at this time: > Bug 3953 Need protocol in A/I webserver/client > Bug 3954 Should limit queue sizes for programs interacting with AI_database > Bug 3955 Please use sanity in setting and getting variables from classes > > Please let me know what you think. > Thank you, > Clay > > On Wed, 15 Oct 2008, Dave Miner wrote: > >> 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 >> _______________________________________________ >> 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
