Shawn Walker wrote:
Brock Pytlik wrote:
Shawn Walker wrote:
Brock Pytlik wrote:
line 1028: An api_errors.CertificateError could happen here for a
remote search on an HTTPS server, right?
Doesn't appear that's the case. Should cert validity be being checked
here? How do I check cert validity for a single server? For one the
user has passed in on the command line?
As we discussed earlier on irc today, you should be checking the cert
validity before attempting to perform a transport operation using the
cert information. You can do that using misc.validate_ssl_cert() and
there are examples of how in src/client.py among other places.
Yeah, wrote the email then talked to you, forgot to erase this bit.
--------------------
modules/indexer.py
--------------------
..
lines 164-170: are there any concerns about the size of this file
and memory footprint since the whole file is being read into memory?
Nope. Right now it's fixed at 128M. I plan in a subsequent wad to
make this adjustable via an environment variable or command line
argument.
That's rather concerning given our target memory environment of 512MB.
On a client, we don't get near 128M. On the server, we limit it to 128M.
It's still a vast improvement on memory usage than we have had. As I
said, environment variable is coming. Considering we're reducing memory
usage, I don't think this is an issue. On my computer, with 730 packages
installed, I rebuild the index using only 81M, down significantly.
lines 544-545: This should raise an ApiException instead of a
RuntimeError.
It can't raise an ApiException, you're in code common to the server
and client here. Also, the only way to hit this exception is for a
caller of this function, who lives inside the api, to pass in an
unknown value. I think this should remain a reuntime error. I can
change it to an assert if you prefer.
The problem with a RuntimeError or AssertionError is that it doesn't
allow the client to cleanly exit with an appropriate message and it
doesn't allow callers to choose to ignore the error because they can't
tell specifically what error case occurred.
As such, if you can't use an api_error here, you should consider
creating a custom exception and then having the calling client api
code re-raise the exception as one of its own so that the client can
exit cleanly. Even if this case should never happen, our client
should not traceback when encountering known error cases.
I disagree. If it's going to be an issue, I'll change the code to check
one value and default to the other. I put the assertion in as a check
against future versions. I don't think adding more exception pumbling
around this bit of code is appropriate.
--------------------
modules/search_storage.py
--------------------
lines 149-150, 249: should raise an ApiException; not RuntimeError
Can't raise an apiException here, for same reason as above. I will
change these to throw other exceptions and catch and rethrow in the
api code.
See above.
As I said, changed.
--------------------
modules/server/depot.py
--------------------
line 263: please don't assert; raise a BadRequest error instead
and use cherrypy.log() to log something meaningful
Why log something?
I assume that it's Really Bad(tm) if this fails and the repository
administrator should now? If not, you don't need to log anything.
Nah, just means a client is doing something dumb. Repository admin
couldn't do anything about it.
Cheers,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
