← Back to team overview

drizzle-discuss team mailing list archive

Re: file system storage engine

 

Hi Stewart,

Thanks for the review :-)
Replies are inline.

On 24/06/2010 3:38 PM, Stewart Smith wrote:
On Fri, 18 Jun 2010 13:45:54 +0800, ZQ<ziminq@xxxxxxxxx>  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.

thanks for reminding. so I need as well to add these checks for write(2) and read(2).
it seems it's a convention to check the return value against EINTR.
- 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.

OK.
- FilesystemCursor::openUpdateFile()
   - on startup you should remove the .UPDATE file if it exists

Ah, I forgot this point. :(
- 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.

storing '0' for NULL is temporary step, because I'm not sure which one is better. defaulting to NULL, while this field is actually an INT, this might be a problem.

- doInsertRecord()
   - you should check the return code of write(2) lots of interesting
     things can go wrong.

same as close(2), will check it.
- ::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)

I'm checking the file permission in validateCreateTableOption() because I want to do a quick fail over,
if the corresponding file doesn't exist, the table can't be created.
doing this check in ::doCreateTable is ok, as long as we permit user can create the table even the file doesn't exist.
- FilesystemEngine::FilesystemEngine()
   - the mutex is destroyed in the destructor, it should be created in
     the constructor.

I did it in plugin's init function 'filesystem_init_func'.
But creating it in constructor and destroying it in destructor seems more natural.
- ::get_share()
   - use an ENUM for share->separator mode instead of number.

OK.
I hope to have a closer look at the update/deletion code shortly.

This comes from my previous refactor of CSV code. I like that idea.
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.

Will look into this 'load data infile'.

--Zimin



References