You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by ba...@apache.org on 2015/07/21 19:28:14 UTC
svn commit: r1692177 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Author: baedke
Date: Tue Jul 21 17:28:14 2015
New Revision: 1692177
URL: http://svn.apache.org/r1692177
Log:
OAK-3131: DocumentNodeStore doesn't serialize blobs that have been created in a different store
BlobSerializer will now also check the origin store.
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/blob/BlobStoreBlob.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/blob/BlobStoreBlob.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/blob/BlobStoreBlob.java?rev=1692177&r1=1692176&r2=1692177&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/blob/BlobStoreBlob.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/blob/BlobStoreBlob.java Tue Jul 21 17:28:14 2015
@@ -74,6 +74,10 @@ public class BlobStoreBlob implements Bl
return blobId;
}
+ public BlobStore getBlobStore() {
+ return blobStore;
+ }
+
//------------------------------------------------------------< Object >--
@Override
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java?rev=1692177&r1=1692176&r2=1692177&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStore.java Tue Jul 21 17:28:14 2015
@@ -342,8 +342,14 @@ public final class DocumentNodeStore
private final BlobSerializer blobSerializer = new BlobSerializer() {
@Override
public String serialize(Blob blob) {
+
if (blob instanceof BlobStoreBlob) {
- return ((BlobStoreBlob) blob).getBlobId();
+ BlobStoreBlob bsb = (BlobStoreBlob) blob;
+ BlobStore bsbBlobStore = bsb.getBlobStore();
+ //see if the blob has been created from another store
+ if (bsbBlobStore != null && bsbBlobStore.equals(blobStore)) {
+ return bsb.getBlobId();
+ }
}
String id;
Re: svn commit: r1692177 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Chetan Mehrotra <ch...@gmail.com>.
On Wed, Jul 22, 2015 at 8:01 PM, Manfred Baedke
<ma...@gmail.com> wrote:
> verifying that the BlobStoreBlob in question comes from this very instance.
> It shouldn't use equals(), though.
Makes sense. Lets discuss this via patch on the bug itself then!
Chetan Mehrotra
Re: svn commit: r1692177 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Manfred Baedke <ma...@gmail.com>.
To elaborate on that: If the current design is assuming exactly one
BlobStore instance per NodeStore instance, the new code does the right
thing - verifying that the BlobStoreBlob in question comes from this
very instance. It shouldn't use equals(), though.
On 22.07.15 14:33, Manfred Baedke wrote:
> A test case will come.
> Isn't the current design assuming that there is exactly one BlobStore
> per NodeStore (rather than a real singleton)?
>
> On 22.07.15 13:54, Chetan Mehrotra wrote:
>> On Wed, Jul 22, 2015 at 4:33 PM, Manfred Baedke
>> <ma...@gmail.com> wrote:
>>> which is just as broken in IMHO and failed to write blobs in Oak2Oak
>>> migration scenarios.
>> Thats why I would like to have a testcase for this. So far current
>> designed assumed that BlobStore is singleton. Supporting blobs from
>> multiple BlobStore would impact other places also.
>>
>>> I would also consider two blobs to be equal iff they contain the
>>> same binary content, which is also not the contract we use for
>>> BlobStoreBlob.
>> Have a look at AbstractBlob#equal on what is considered as a blob
>> equality contract which takes care of that. So probably we use that
>> here
>>
>> So if we plan to support such case then we cleanup this part first and
>> then fix it.
>>
>> wdyt?
>>
>> Chetan Mehrotra
>
Re: svn commit: r1692177 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Manfred Baedke <ma...@gmail.com>.
A test case will come.
Isn't the current design assuming that there is exactly one BlobStore
per NodeStore (rather than a real singleton)?
On 22.07.15 13:54, Chetan Mehrotra wrote:
> On Wed, Jul 22, 2015 at 4:33 PM, Manfred Baedke
> <ma...@gmail.com> wrote:
>> which is just as broken in IMHO and failed to write blobs in Oak2Oak
>> migration scenarios.
> Thats why I would like to have a testcase for this. So far current
> designed assumed that BlobStore is singleton. Supporting blobs from
> multiple BlobStore would impact other places also.
>
>> I would also consider two blobs to be equal iff they contain the same binary content, which is also not the contract we use for BlobStoreBlob.
> Have a look at AbstractBlob#equal on what is considered as a blob
> equality contract which takes care of that. So probably we use that
> here
>
> So if we plan to support such case then we cleanup this part first and
> then fix it.
>
> wdyt?
>
> Chetan Mehrotra
Re: svn commit: r1692177 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Chetan Mehrotra <ch...@gmail.com>.
On Wed, Jul 22, 2015 at 4:33 PM, Manfred Baedke
<ma...@gmail.com> wrote:
> which is just as broken in IMHO and failed to write blobs in Oak2Oak
> migration scenarios.
Thats why I would like to have a testcase for this. So far current
designed assumed that BlobStore is singleton. Supporting blobs from
multiple BlobStore would impact other places also.
> I would also consider two blobs to be equal iff they contain the same binary content, which is also not the contract we use for BlobStoreBlob.
Have a look at AbstractBlob#equal on what is considered as a blob
equality contract which takes care of that. So probably we use that
here
So if we plan to support such case then we cleanup this part first and
then fix it.
wdyt?
Chetan Mehrotra
Re: svn commit: r1692177 - in
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Manfred Baedke <ma...@gmail.com>.
Hi Michael, Chetan,
I absolutely agree that this code has to improve. Before the change it read
if (blob instanceof BlobStoreBlob) {
return blob.getBlobId();
}
which is just as broken in IMHO and failed to write blobs in Oak2Oak
migration scenarios.
> I would consider to blob stores to be equal iff they contain the same
> blobs.
I would also consider two blobs to be equal iff they contain the same
binary content, which is also not the contract we use for BlobStoreBlob.
On 22.07.15 10:17, Michael Dürig wrote:
>
> On 22.7.15 5:39 , Chetan Mehrotra wrote:
>> Hi Manfred,
>>
>> On Tue, Jul 21, 2015 at 10:58 PM, <ba...@apache.org> wrote:
>>> + if (bsbBlobStore != null &&
>>> bsbBlobStore.equals(blobStore)) {
>>> + return bsb.getBlobId();
>>> + }
>>
>> Can we have a testcase for this scenario? So far we do not have
>> requirement to support equality for BlobStore instance. So would like
>> to understand the usecase preferably with a testcase. May be the
>> problem affect SegmentNodeStore also (not sure though)
>
> I double this! Please use other means for checking the origin of a
> blob. Equality is not the right thing here. I would consider to blob
> stores to be equal iff they contain the same blobs. But then above
> condition would be wrong.
>
> Michael
Re: svn commit: r1692177 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Michael Dürig <md...@apache.org>.
On 22.7.15 5:39 , Chetan Mehrotra wrote:
> Hi Manfred,
>
> On Tue, Jul 21, 2015 at 10:58 PM, <ba...@apache.org> wrote:
>> + if (bsbBlobStore != null && bsbBlobStore.equals(blobStore)) {
>> + return bsb.getBlobId();
>> + }
>
> Can we have a testcase for this scenario? So far we do not have
> requirement to support equality for BlobStore instance. So would like
> to understand the usecase preferably with a testcase. May be the
> problem affect SegmentNodeStore also (not sure though)
I double this! Please use other means for checking the origin of a blob.
Equality is not the right thing here. I would consider to blob stores to
be equal iff they contain the same blobs. But then above condition would
be wrong.
Michael
Re: svn commit: r1692177 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Chetan Mehrotra <ch...@gmail.com>.
Also note that its possible that later we may be wrapping the
BlobStore instance. For example add a wrapper for monitioring and in
such a case this equality condition might fail. A more stable fix
would be to check with registered BlobStore weather it knows the given
blobId or not.
Chetan Mehrotra
On Wed, Jul 22, 2015 at 9:09 AM, Chetan Mehrotra
<ch...@gmail.com> wrote:
> Hi Manfred,
>
> On Tue, Jul 21, 2015 at 10:58 PM, <ba...@apache.org> wrote:
>> + if (bsbBlobStore != null && bsbBlobStore.equals(blobStore)) {
>> + return bsb.getBlobId();
>> + }
>
> Can we have a testcase for this scenario? So far we do not have
> requirement to support equality for BlobStore instance. So would like
> to understand the usecase preferably with a testcase. May be the
> problem affect SegmentNodeStore also (not sure though)
>
> Chetan Mehrotra
Re: svn commit: r1692177 - in /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins:
blob/BlobStoreBlob.java document/DocumentNodeStore.java
Posted by Chetan Mehrotra <ch...@gmail.com>.
Hi Manfred,
On Tue, Jul 21, 2015 at 10:58 PM, <ba...@apache.org> wrote:
> + if (bsbBlobStore != null && bsbBlobStore.equals(blobStore)) {
> + return bsb.getBlobId();
> + }
Can we have a testcase for this scenario? So far we do not have
requirement to support equality for BlobStore instance. So would like
to understand the usecase preferably with a testcase. May be the
problem affect SegmentNodeStore also (not sure though)
Chetan Mehrotra