Clay Baenziger wrote: > Hi Dave, > My comments are inline. Thank you for taking time to look at this > again; hopefully it's starting to look a bit better. > > We're now up to: > http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc5 >
Much better, see things in-line below. > -Clay > > On Thu, 16 Oct 2008, Dave Miner wrote: > >> Clay Baenziger wrote: >>> Hi Dave and Jack, >>> You both raised concerns with the response(), manifestPath(), >>> service() and database() functions in AI_database, and publish_manifest >>> which did not have explicit set and accessor functions. I've have >>> corrected that in: >>> http://cr.opensolaris.org/~clayb/AI_server/webrev_final_rc3/. >>> >>> Thank you, >>> Clay >> Long functions (e.g. run, getCriteria) continue to need more vertical >> space. Think of it like paragraphs: you don't run sentence after >> sentence together in prose without paragraph breaks, nor should you do >> so in code. Comments don't provide visual separation, they're part of >> the code. Loops and conditional blocks are usually good breaking points >> in the narrative. My review time was definitely increased due to this. >> >> So what's the AI.db file, and what's in it? Is it something we should >> be generating in the build rather than checking in as binary? >> Need to answer above question, please. >> AI_database.py >> >> 131: usually predicates are better named using isFoo; in this case, >> isFinished. > > I agree, I had waitAns, and get/setResponse so now I have isFinished, and > getSql, and needsCommit > >> 260: comment here doesn't appear to match the code, as all requests are >> done in the loop. > > That's more of a housekeeping comment (perhaps it should go). I don't know > which is more efficient right now (haven't had time to check) but wanted > to point out that we could do either (iterate doing DB lookups or iterate > over the result of one big DB lookup) with associated pros/cons. > The comment as it is written implies that there's two different queries going on. Changing "is" to "should be" would cover the issue, I think. >> 303: indent level looks wrong here > > Yes it was, and should be better now > >> 353: out of curiosity, any reason you prefer == False to the not operator? > > I guess just to be pedantic. onlyUsed could be false if it were set to > None or "", but those would be errors (admittedly unhandled). > >> 446: I don't quite see how we'd get a LookupError out of this block. > > Thank you, I intended a KeyError, I think at one point that wasn't using a > dict(). > >> 458: error message is still violating layers > > Thank you; I missed that one; now same as above. > >> delete-manifest.py >> >> General comment: the structure of the command line here seems to prevent >> deleting multiple manifests with one call into it. If so, we should >> reconsider this. > > Yes, it was modeled off installadm's design doc, which has a similar > limitation. I don't know if optparse supports repeating options, but if > so, we could require an option for the manifest and repeat over each -i > and (whatever we use for the manifest). I think we'd have to look at using > a different option parser, however. > OK for now. Other UI's might well offer a batch deletion, so when you're doing what's essentially a library behind a UI, it's a good idea to consider its reusability for alternate UI's. >> General comment 2 (for here and elsewhere): Seems to be excessive use of >> SystemExit in what are essentially library functions. This impedes >> their re-use within alternate user interfaces. Consider >> application-specific exceptions instead, which allow for better >> separation of error handling. > > I agree about SystemExit being used a lot. I would like to architect a > better exception structure. If they're being reused in a python > application their SystemExits can be caught for application specific error > handling at least (as I've done in publish_manifest catching SystemExit). > OK, please file a bug on this so we come back to it. >> 73: I'm not following how the elements of manifestInstance are being >> used; why are we interested in [1] here? A comment outlining the theory >> is appropriate if the code is correct. > > Yes that's checking if instance is set the instance part of > manifestInstance (a two element list with manifest and instance). It's a > Clay convention, so I've set instance at the top and changed the body. > > Otherwise, the code is doing as the comment says: "# if we do not have an > instance" though as that's just a fragment I've changed it to read "# if > we do not have an instance remove the entire manifest" > >> 86,110: This seems like a warning. The end state is the desired one, >> though something was out of sync here. > > Sure I can see that. They've been changed to sys.err print outs. (Also, to > keep database operations together I moved the block from 105-111 down) > Using the term Error in what you print implies that there's something to be fixed; suggest perhaps "Warning: file %s was already removed" or something of that nature. >> 112: I'm not understanding the reason for this block at all. A comment >> explaining is needed. > > I've updated the block's comment to be: > # may need to reshuffle manifests to prevent gaps in instance numbering > # as we just removed an instance which may have been in the middle > > I've also further commented the block with two more comments > I guess I don't quite follow why we need to number the instances here, or why a gap is problematic. This is where a big block comment somewhere about the theory of the database structure would be helpful. >> list-manifests.py >> >> vertical spacing, please > > More spacing and a few more comments (with some bug fixes from them) > >> publish-manifest.py >> >> vertical spacing, please > > Lots more > >> 291: this is really dense, and seems to do the heavy work before the >> simple. Consider something like (pseudocode for clarity): > > Dave thank you for reminding me. Pseudocode can be great for > documentation. I don't however see how best to employ it here, I've mostly > copied your example. > The suggestion was to restructure the code along this line, sorry that wasn't clear. Trying to avoid having a long statement like the collisions... repeated multiple times, since it makes the code really hard to parse. >> if dbcrit != mancrit >> if min or max: >> if collision >> raise error >> >> >> 303: why case-insensitive here and not at 295? > > Range test versus a string test, however, for safety's sake I've put lower > on the range too (i.e. if a hex string comes in funky and for code > symmetry) > >> 334: why skip min's? > > Arbitrary we just handle ranges together. (I've now commented as such.) > >> 394: "correct" > > Thanks, I know crossing t's is important but writing them at all is too > :) > >> 405ff: indents start getting loopy again. Please pay attention to this, >> nothing annoys this reviewer more than continuing to see problems in a >> second round that were already raised once. > > Sorry this programmer must have gone loopy. I think I've gotten them all > now, really I do > >> 434: why 555 not 755? > > Didn't really see the necessity of making it writable at first, but it > seems it would be wise. How about 644 though, as no one should want to > execute their XML? > That's fine, just fix the comment at 480 to match. >> 702: garbage in comment here > > Thank you > >> verifyXML.py >> >> 39: indentation > > This was left out of the great tabification, should be fixed now > >> webserver.py >> >> >> 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. > > Okay can we make up our own MIME type or who do we need to register it > with, similarly, the likelyhood of a server implementing the correct > criteria handshake (i.e. accepting a POST then allowing a GET) on > /manifest.xml seems pretty low to me, and like someone would have to > intentional do it. > Fair point. Leave it, I suppose. >> usr/src/cmd/Makefile: >> 36: Alphabetize lists such as this. This also hasn't been merged >> against the current version yet. > > Thank you, makes sense > >> 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. > > When do we consider a line too long? I was trying to wrap these to fit ~80 > columns, but I have three heads horizontally if need be :) > > I've since set these to be vertically aligned as that seems to be > precedent? > > I certinaly understand how nice a uniform style is, especailly as code > grows! > 80 columns is the width to use. But you're using continuations when they aren't needed (39, 42). Generally, if we use a structure such as you have at 49 for PYTHON_EXECS, we do it so that the list can be easily alphabetized by the editor (or pasting through sort if your editor lacks built-in sorting). Excessive space after the = such as at 54 makes it harder to read, not easier, since alignment is lost visually with that much white space interceding. Usually, the actions for targets (83ff) are only indented a single tab, since they can be long command lines. >> 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. > > This has been fixed > Looks like it's still off to me. >> 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? > > I don't have much knowledge about the A/I client side I think Sundar or > Alok would be the one to ask > Let's get that answered. >> 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. > > Yes, there is /usr/share/xml/rng however, there's the possibility of A/I > service specific versioning of the manifests. Perhaps > /usr/share/xml/rng/osol-install/auto-install/<version>? This is more > Sundar's call likely too. > /var is wrong, though, see filesystem(5) for guidance on what should be in /var. Dave
