Akiban Server

Code review comment for lp:~tjoneslo/akiban-server/file-merge-sort

file-merge-sort
Merge into trunk

Revision history for this message

Thomas Jones-Low (tjoneslo) wrote on 2013-07-28:

> Nice! This looks pretty good overall. A handful of small issues below.
>
> The exceptions on lines 166 and 192 probably needs bubbled up.
>
Done

> Line 291, 306 and 319 look suspicious. InputStream returns -1 at EOF, whereas
> this is checking read < size. Maybe an assert instead?
>
Converted to an assert.

> Line 491, a new config instead of persistit.tmpvoldir might be nice. Or
> perhaps an akserver.tmp_dir, which TreeService can then assign to tmpvoldir if
> otherwise unset.
>
tmpvoldir is a persistit specific configuration item, the only way to alter the manner in which it gets set is to make changes to Persistit.

Added the akserver.tmp_dir variable and use that instead.

> Did you investigate if the fasterxml.Sorter closes all of the temp files from
> the provider on finish? On error? If not, I wonder if we need to hang onto
> them all and ensure they are in close().
>
The initial read into the temp files does so correctly. The merge stage does not, and I've filed a bug in the Merge-sort github. https://github.com/cowtowncoder/java-merge-sort/issues/8

> Line 496, could SimplQueryContext look at adapter.getSession().sessionId()
> instead of throwing? Or is there no Session either?
>

Fixed, removed the try/catch in MergeJoinSorter too. The adapter session sessionIDs and the context sessionIDs are different creatures (the former is from SessionService, the latter are from the MontiorService) but for these purposes they can be the same.

> Line 550, is that TODO relevant or just stale?
>
It's relevant. I still don't have an answer for that.

> Line 576, I think we just have to eat that one. Maybe log for... good measure?
>

> TestKeyReaderWriter.random being static and unseeded makes me a little
> nervous. A non-deterministic IT is asking for headaches.
>

Fixed.

> Does surefire actually pick up TestKeyReaderWriter? I think our pattern only
> looks for *Test.java.
>

Renamed to KeyReaderWriterTest

> Maybe a new configuration that get shimmed into SortConfig? I can imagine
> wanting to set that pretty quickly.

The only other configuration item is the size of the sort buffer. Added a akserver.sort.memory configuration item and set the default to 64M.

> Nice! This looks pretty good overall. A handful of small issues below.
> 
> The exceptions on lines 166 and 192 probably needs bubbled up.
> 
Done

> Line 291, 306 and 319 look suspicious. InputStream returns -1 at EOF, whereas
> this is checking read < size. Maybe an assert instead?
> 
Converted to an assert.

> Line 491, a new config instead of persistit.tmpvoldir might be nice. Or
> perhaps an akserver.tmp_dir, which TreeService can then assign to tmpvoldir if
> otherwise unset.
> 
tmpvoldir is a persistit specific configuration item, the only way to alter the manner in which it gets set is to make changes to Persistit.

Added the akserver.tmp_dir variable and use that instead.

> Did you investigate if the fasterxml.Sorter closes all of the temp files from
> the provider on finish? On error? If not, I wonder if we need to hang onto
> them all and ensure they are in close().
> 
   The initial read into the temp files does so correctly. The merge stage does not, and I've filed a bug in the Merge-sort github. https://github.com/cowtowncoder/java-merge-sort/issues/8