You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "Esteban Franqueiro (JIRA)" <ji...@apache.org> on 2008/02/15 18:46:07 UTC
[jira] Created: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
-------------------------------------------------------------------------------------
Key: JCR-1388
URL: https://issues.apache.org/jira/browse/JCR-1388
Project: Jackrabbit
Issue Type: Improvement
Components: jackrabbit-core
Affects Versions: 1.4
Environment: WinXP x64, Eclipse, remote SQL Server 2005
Reporter: Esteban Franqueiro
Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: JCR-1388-datastore-concurrent-reads.9.patch
> As I wrote before, I suggest you use Checkstyle so that you can find such problems yourself.
I'm using it, but I missed those.
> What is the reason for the following code?
> ...
> Why do you close i1 and i2 twice? Why do you check for null?
Oops my mistake. Fixed that. I left the try/catch calls, so that it logs the exception. Should I re-throw it?
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.9.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: JCR-1388-datastore-concurrent-reads.10.patch
Oops, wrong file. Here is the right one.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Thomas Mueller resolved JCR-1388.
---------------------------------
Resolution: Fixed
Fix Version/s: 1.5
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Fix For: 1.5
>
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12573307#action_12573307 ]
Esteban Franqueiro commented on JCR-1388:
-----------------------------------------
Hi.
Did anyone take a look at this patch?
Regards
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.patch
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12576912#action_12576912 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
Hi,
In TestTwoGetStreams there are many 'if' without {}:
if (i2 != null)
i2.close();
According to the code style, this should be written
if (i2 != null) {
i2.close();
}
As I wrote before, I suggest you use Checkstyle so that you can find such problems yourself.
What is the reason for the following code?
i1.close();
i2.close();
try {
if (i1 != null)
i1.close();
} catch (IOException e) {
log.info("Could not close first input stream: ", e);
}
try {
if (i2 != null)
i2.close();
} catch (IOException e) {
log.info("Could not close second input stream: ", e);
}
Why do you close i1 and i2 twice? Why do you check for null?
The line
private final static int BLOCK_SIZE = 4 * 1024;
should be (according to Checkstyle):
private static final int BLOCK_SIZE = 4 * 1024;
Your method assertEquals is incorrect:
while (n1 != -1 || n2 != -1) {
n1 = i1.read(b1);
n2 = i2.read(b2);
for (int i = 0; i < n1; i++) {
assertEquals(message + "; byte #" + i + " mismatch!", b2[i], b1[i]);
}
count1 += n1;
count2 += n2;
}
i1 and i2 may not return the same number of bytes (n1 != n2). I suggest to test byte by byte.
Otherwise everything looks very good to me.
Regards,
Thomas
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: TestTwoGetStreams.java
JCR-1388-datastore-concurrent-reads.8.patch
Here's a new version. I removed the synchronization from DbInputStream. I'm also posting a new version of the test, and the previous, fixed, version in the patch.
Regards
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12579497#action_12579497 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
Hi,
Sorry for the delay... I think there is one potential problem in assertEquals(String message, InputStream i1, InputStream i2):
+ int b1 = 0, b2 = 0;
+ int i = 0;
+ while (b1 != -1 || b2 != -1) {
+ b1 = i1.read();
+ b2 = i2.read();
+ assertEquals(message + "; byte #" + i + " mismatch!", (byte) b2, (byte) b1);
+ ++i;
+ }
This test wouldn't detect a problem if b1 is -1 and b2 is 255. I would change the code to:
assertEquals(message + "; byte #" + i + " mismatch!", b2, b1);
Everything else looks fine to me.
Regards,
Thomas
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12573844#action_12573844 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
Hi,
Your new patch is better, but there are still a few issues:
You still have tabs in the source code.
DbResources and DatabaseHelper still don't have correct license headers.
There is still a catch (Exception e) {}.
There are still some cases where "} else {" and so on is not on the same line. Please format the source code. In Eclipse, use [Source] [Format]. I suggest you review the Sun coding guidelines at http://java.sun.com/docs/codeconv/
Javadoc comments are still not correct. getLastModified doesn't have a Javadoc comment. @return tag is still not used.
Variables are still declared at the start of the method in getDatabaseResources(). Please declare them when / just before using them
DbInputStream.mark and reset are synchronized, but nothing else in this class, why?
The SQL statement remark "// SELECT ID, DATA FROM DATASTORE WHERE ID = ?" was
there to simplify reading the code (so you don't have to switch to another file to understand it).
Please add it where selectDataSQL is used.
About the method finalize(): I wouldn't use it. It slows down creating objects. If the wrapped InputStream and resource need finalize(), it is already implemented there.
About the test cases:
- ".classpath" doesn't exist in all systems (RandomInputStream is better).
- Don't use System.out.println, use a logger.
- Add the test to the TestAll method.
- You have again used return(..) instead of return ..
- The Javadoc comment is empty
Regards,
Thomas
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: (was: JCR-1388-datastore-concurrent-reads.10.patch)
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12575765#action_12575765 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
Hi,
Your new patch is better, but there are still a few issues:
You still have tabs in the source code: 2 to go ;-)
There is still a catch (Exception e) {}. catch (IOException e) {} should at least have a remark, but it's better to log the exception (with stack trace)
TestTwoGetStreams.streamToString() expects to read everything with one InputStreamReader.read call. This method could theoretically return a value smaller than requested in length. You should deal with that in some way; for example, use DataInputStream.readFully. Also, the conversion to String expects the data is stored in the current system encoding, that may not be the case.
I wouldn't use new FileInputStream("NOTICE.txt"), I would use new RandomInputStream(0, 10 * 1024 * 1024). You can't be sure that NOTICE.txt will always be there and have that name.
The method testTwoGetStreams doesn't have any assertion. It doesn't check if the data read from the repository is the same as in the files. Only calling log.info is not a good test I believe.
DatabaseHelper doesn't have a class level javadoc.
getDatabaseResources: is the 'finally' and the boolean success required? Could the code in 'finally' not be written in the 'catch' block?
DbInputStream.mark and reset are synchronized: I don't understand why this needs to be synchronized in your class as well. I probably need to ask Arthur van Hoff then ;-)
Regards,
Thomas
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12590418#action_12590418 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
Committed in revision 649493 (trunk)
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Fix For: 1.5
>
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: JCR-1388-datastore-concurrent-reads.patch
This patch addresses the issue by provinding a wrapper input stream that encapsulates the database resources, and not returning the connection to the pool until the stream is consumed/closed.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.patch
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12573815#action_12573815 ]
Esteban Franqueiro commented on JCR-1388:
-----------------------------------------
Hi.
> I have a few remarks, first about the source code 'style'. I use Eclipse and the Checkstyle plugin, this should find most issues:
> - you should use spaces instead of tabs
> - return doesn't required brackets: return(false) should be changed to return false
> - catch (IOException e) {} should at least have a remark, but it's better to log the exception (with stack trace)
> - you need to replace the file file headers
> - use '} else {', '} finally {' and '} catch (Exception e) {' as in the Sun Java coding guidelines
> - Don't declare all variables at the start of the method as in C. Declare them when / just before using them
> (for example, getResourceAsReader(), reader; there are others
> - Review the Javadocs rules (add comments, use the @param, @return tags)
> - Only use this. when required
Fixed all of those I think.
> Some other remarks:
> - I didn't see any test cases - please add one
> - Are prepareSchemaObjectPrefix and getResourceAsReader used somewhere? Don't add unused methods
Removed them.
> - close() methods easting exceptions should be called closeSilently()
Renamed them.
> - You have removed the SQL statement remark, why? // SELECT ID, DATA FROM DATASTORE WHERE ID = ?
Because I deleted the entire method.
> - If you are removing code, remove the lines, don't remark them (+//lastModified = ...)
> - getDatabaseResources, boolean success is always true
> - You hare remarked "lastModified = store.touch(getIdentifier(), lastModified)", why?
Fixed those.
> - Don't swallow exceptions (use IOException.initCause in DbInputStream.getStream())
> - Synchronization is very inconsistent (DbInputStream)
In what sense? The DbInputStream is not supposed to be used from multiple threads, so I only synchronized the methods that are already synchronized in InputStream.
I'll attach the new patch and test.
Regards.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.patch
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12579900#action_12579900 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
+1
The patch looks good to me.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: JCR-1388-datastore-concurrent-reads.4.patch
Ok, here's a new take at it. I hope I resolved all the issues you pointed out.
> DbInputStream.mark and reset are synchronized, but nothing else in this class, why?
I followed the pattern used in InputStream and FilterInputStream.
Regarding the finalizer, I removed it, but I think that it depends on what your position is regarding improper use of the stream. If the policy is to protect against that kind of mistake or to let it happen so that in the end the user has to fix it's application.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Thomas Mueller (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12573737#action_12573737 ]
Thomas Mueller commented on JCR-1388:
-------------------------------------
Hi,
I have a few remarks, first about the source code 'style'. I use Eclipse and the Checkstyle plugin, this should find most issues:
- you should use spaces instead of tabs
- return doesn't required brackets: return(false) should be changed to return false
- catch (IOException e) {} should at least have a remark, but it's better to log the exception (with stack trace)
- you need to replace the file file headers
- use '} else {', '} finally {' and '} catch (Exception e) {' as in the Sun Java coding guidelines
- Don't declare all variables at the start of the method as in C. Declare them when / just before using them
(for example, getResourceAsReader(), reader; there are others
- Review the Javadocs rules (add comments, use the @param, @return tags)
- Only use this. when required
Some other remarks:
- I didn't see any test cases - please add one
- Are prepareSchemaObjectPrefix and getResourceAsReader used somewhere? Don't add unused methods
- close() methods easting exceptions should be called closeSilently()
- You have removed the SQL statement remark, why? // SELECT ID, DATA FROM DATASTORE WHERE ID = ?
- If you are removing code, remove the lines, don't remark them (+//lastModified = ...)
- getDatabaseResources, boolean success is always true
- You hare remarked "lastModified = store.touch(getIdentifier(), lastModified)", why?
- Don't swallow exceptions (use IOException.initCause in DbInputStream.getStream())
- Synchronization is very inconsistent (DbInputStream)
Regards,
Thomas
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.patch
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: TestTwoGetStreams.java
JCR-1388-datastore-concurrent-reads.2.patch
The new patch corrects the issues pointed by Thomas. The test passes with the patch applied, copyWhenReading=false and storeStream=-1. Without the patch, the test fails with the same conditions.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: JCR-1388-datastore-concurrent-reads.10.patch
I agree with you Thomas. Here's the corrected patch.
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: (was: TestTwoGetStreams.java)
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (JCR-1388) Jackrabbit does not allow concurrent
reads to the data store if copyWhenReading=false
Posted by "Esteban Franqueiro (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/JCR-1388?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Esteban Franqueiro updated JCR-1388:
------------------------------------
Attachment: (was: JCR-1388-datastore-concurrent-reads.9.patch)
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false
> -------------------------------------------------------------------------------------
>
> Key: JCR-1388
> URL: https://issues.apache.org/jira/browse/JCR-1388
> Project: Jackrabbit
> Issue Type: Improvement
> Components: jackrabbit-core
> Affects Versions: 1.4
> Environment: WinXP x64, Eclipse, remote SQL Server 2005
> Reporter: Esteban Franqueiro
> Attachments: JCR-1388-datastore-concurrent-reads.10.patch, JCR-1388-datastore-concurrent-reads.2.patch, JCR-1388-datastore-concurrent-reads.4.patch, JCR-1388-datastore-concurrent-reads.8.patch, JCR-1388-datastore-concurrent-reads.patch, TestTwoGetStreams.java
>
>
> Jackrabbit does not allow concurrent reads to the data store if copyWhenReading=false, even if maxConnections>1.
> See JCR-1184 for a test for this problem (run it with copyWhenReading=false).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.