On Fri, 18 Jun 2010 13:45:54 +0800, ZQ <[email protected]> wrote:
> could you please do some review over my branch? It would be helpful.
Sorry this has taken a little while to get back to you, I've been in
training all week. Hope you were still able to get some things done.
Cool! I've spotted a few things for you to take a look at:
- FilesystemCursor::close should check the return value of close(),
especially for EINTR.
- on non-EINTR errors, you can probably just return them and kind of
hope that the upper layer reports these correctly. Since with buffered
IO close(2) can also return all the errors that write() can, things
like EIO may occur here.
- FilesystemCursor::position()
- avoid using things in the drizzled::internal namespace. Instead you
can just cast ref to an off_t and store it in there.
- FilesystemCursor::openUpdateFile()
- on startup you should remove the .UPDATE file if it exists
- getAllFields() is a bit misleadingly named. Perhaps recordToString() ?
- is storing '0' for NULL a good idea? Perhaps another engine
option. defaulting to 'NULL' is probably better too.
- doInsertRecord()
- you should check the return code of write(2) lots of interesting
things can go wrong.
- ::validateCreateTableOption()
- instead of validating access to the file in
validateCreateTableOption (which can only return 'success' or
'failure'), check this in ::doCreateTable which can return a real
error message (e.g. ENOENT, EPERM, EIO etc)
- FilesystemEngine::FilesystemEngine()
- the mutex is destroyed in the destructor, it should be created in
the constructor.
- ::get_share()
- use an ENUM for share->separator mode instead of number.
I hope to have a closer look at the update/deletion code shortly.
hopefully this helps. things are looking pretty good. One idea may be to
go through the 'load data infile' tests, copy them and rewrite parts to
use the filesystem engine instead.
cheers,
stewart
--
Stewart Smith
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
