db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Knut Anders Hatlen (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-2247) provide set methods for blob in embeded driver
Date Mon, 21 May 2007 13:56:17 GMT

    [ https://issues.apache.org/jira/browse/DERBY-2247?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12497456
] 

Knut Anders Hatlen commented on DERBY-2247:
-------------------------------------------

>>1) Wouldn't it be simpler to write
>> BaseStorageFactory.createTemporaryFile() in terms of
>> File.createTempFile(String prefix, String suffix, File directory)?
>
>I thought it may be useful in case some other module decides to use
>this same service and want to generate file name based of some
>pattern.

BaseStorageFactory.createTemporaryFile() could still be used by other
modules if it was written in terms of File.createTempFile(), but the
implementation of the method would be simpler, like this:

    public StorageFile createTemporaryFile(String prefix, String suffix)
        throws IOException
    {
        return File.createTempFile(prefix, suffix,
                                   new File(getTempDir().getPath()));
    }

This would also remove the need for the counter variable in
BaseStorageFactory. The only difference would be that the prefix no
longer could be null, but that should be easy to fix.

>>5) LOBInputStream.read(byte[]) and read(byte[],int,int) contain this
>> piece of code:
>>+ int ret = control.read(b, off, len, pos);
>>+ if (ret > 0) {
>>+ pos += ret;
>>+ return ret;
>>+ }
>>+ return -1;
>>Since a call to InputStream.read(byte[]...) theoretically can return
>>0, I think (ret > 0) should be changed to (ret >= 0) or perhaps (ret
>>!= -1).
>
>this was taken care by one of the privious patch

The previous patch removed read(byte[]), but read(byte[],int,int) is
still there with the same code.

>>11) LOBStreamControl.init() might ignore some exceptions.
>
>Added all skipped exception in unexpected exception.

I'm not sure unexpectedUserException() is the correct method to use to
wrap the exception in this case. I thought it was supposed to be used
to wrap exceptions thrown by user code (like stored procedures and
VTIs). Perhaps Util.javaException() would be better?

>>1) Can control be final?
>
>ClobStreamControl extends from LOBStreamControl. ClobStreamControl is
>already final.

I think Kristian was referring to the variable control in
LOBOutputStream. It is not final.

>>10) I think it would be good to split the testSetBytes into one test
>>method for in-memory operation, and one for on disk operation.
>
>fixed

It would also be good if these test cases called close() on
rs. Another thing I noticed is that the test cases contain clean-up
code in a finally block. If an exception is thrown in the finally
block, it will hide the original error, so I think it's better to
remove the finally block.

>>15) Comment why the test is currently only run in embedded in suite().
>
>these tests, the way they are written, are specific to embedded
>(testing with memory and then file system) and the client already has
>similar tests in jdbcapi/LobStreamsTest.

Could you add this comment in the source file as well? Would it run in
client/server mode? If so, I think it would be valuable to run it
under the client too.

Some other comments to LobStreamTest:

  - tearDown() doesn't call super.tearDown() (if it did, calling
    rollback() and close() on conn wouldn't be necessary)

  - the instance variables dbName, useLOBStreamControl and f are not
    used and should be removed

  - the instance variable conn is not needed since the connection is
    also stored in the super class

  - blob should be set to null in tearDown()

>Follow up comments from knuth
>
>>- long finalLen = (dataBytes != null) ? dataBytes.length + b.length
>>- : b.length;
>>- if (finalLen < MAX_BUF_SIZE)
>>+ if (pos + b.length < MAX_BUF_SIZE)
>> return updateData(b, off, len, pos);
>> else {
>> init(dataBytes, pos);
>> }
>
>>Shouldn't b.length have been len, in case we don't write the entire byte array?
>>And shouldn't the comparison use <= instead of <?
>
>changed

I noticed that you didn't change the comparison operator. Do you think
that comment was wrong?

> provide set methods for blob in embeded driver
> ----------------------------------------------
>
>                 Key: DERBY-2247
>                 URL: https://issues.apache.org/jira/browse/DERBY-2247
>             Project: Derby
>          Issue Type: Sub-task
>          Components: JDBC
>         Environment: all
>            Reporter: Anurag Shekhar
>         Assigned To: Anurag Shekhar
>            Priority: Minor
>         Attachments: derby-2247-followup.diff, derby-2247-followup2.diff, derby-2247-followup2.diff,
derby-2247-v3-usingStoreFactory.diff, derby-2247-v4-usingStoreFactory.diff, derby-2247.diff,
derby-2247v2-using_StoreFactory.diff
>
>


-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message