[jira] Commented: (DERBY-2247) provide set methods for blob in embeded driver

["V.Narayanan (JIRA)" <ji...@apache.org>]

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

V.Narayanan commented on DERBY-2247:
------------------------------------

Anurag's code looks very good. My comments are mostly nits.

java/engine/org/apache/derby/impl/jdbc/LOBStreamControl.java
------------------------------------------------------------

Pls mention reason for choosing buffer size as 4096, if there was a particular
reason for choosing this value. Otherwise pls mention that it was a random
value. 

init - line nos 72 and 74
----

Can the Monitor and the DataFactory be resused instead of doing a findService,
findServiceModule each time?

line no 77
----------
String str = df.getStorageFactory().getTempDir().getPath(); (str is not being
used anywhere)

line nos 171,196,229
--------------------

write methods

I notice that you have repeated the code snippet

		if (isBytes) {
            if (pos + 1 < MAX_BUF_SIZE) {
                byte [] bytes = {(byte) b};
                updateData(bytes, 0, 1, pos);
                return pos + 1;
            } else {
                init(dataBytes, pos);
            }
        }
        if (tmpFile.getFilePointer() != pos)
            tmpFile.seek(pos);
        tmpFile.write(b);
        return tmpFile.getFilePointer();

in all the three set methods. Is it possible that you can combine the code into one method that can
be called from all the three set methods?

java/engine/org/apache/derby/impl/jdbc/EmbedBlob.java
-------------------------------------------------------
can materialized can be initialized to false during declaration?
can control be initialized to null during declaration?
set methods
-----------
you check the following in the set methods

 if (pos - 1 > length())
	throw Util.generateCsSQLException(
		SQLState.BLOB_POSITION_TOO_LARGE, new Long(pos));
 if (pos < 1)
	throw Util.generateCsSQLException(
		SQLState.BLOB_BAD_POSITION, new Long(pos));

But would'nt isValidPostion(pos) in control.write take care of this and throw
appropriate exception? 

truncate
-------
can you move length checks inside truncate methods into control class similar to
pos checks?

Some general comments
---------------------
There are a few missing javadocs and some javadocs missing the @throws tags.


> 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.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.