Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644787553
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -60,6 +73,11 @@ public void before() throws Exception {
ns = builderProvider.newBuilder().setBlobStore(bs).getNodeStore();
}
+@After
+public void tearDown() {
+ns.dispose();
Review Comment:
```suggestion
try {
ns.dispose();
} finally {
DocumentPropertyState.setCompressionThreshold(DISABLED_COMPRESSION);
}
```
to avoid side-effects, the test should ensure it doesn't leak a non default
compression threshold to other tests. DISABLED_COMPRESSION is for illustration
to refer to -1 - ideally the -1 would only be defined in the source class...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on PR #1526: URL: https://github.com/apache/jackrabbit-oak/pull/1526#issuecomment-2176272288 [looks](https://github.com/apache/jackrabbit-oak/blob/14c357120e49d83281dc473d810ca485deb94501/oak-store-spi/src/main/java/org/apache/jackrabbit/oak/plugins/memory/AbstractPropertyState.java#L103-L106) like the original intention was that properties shouldn't or wouldn't be used as keys in hash maps... if that's the case then it should be fine.. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644584397
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeRepositoryTest.java:
##
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.JcrConstants;
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeRepositoryTest extends AbstractRepositoryTest {
+
+public MoveVersionableNodeRepositoryTest(NodeStoreFixture fixture) {
+super(fixture);
+}
+
+@Parameterized.Parameters(name="{0}")
+public static Collection memoryFixture() {
+return NodeStoreFixtures.asJunitParameters(singleton(DOCUMENT_NS));
+}
+
+/**
+ * Creates a versionable node with the specified name under the given
parent node, then
+ * saves the session.
+ * @param parent
+ * @param nodeName
+ * @return
+ * @throws Exception
+ */
+private Node createVersionableNode(Node parent, String nodeName) throws
Exception {
+Node newNode = (parent.hasNode(nodeName)) ? parent.getNode(nodeName) :
parent.addNode(nodeName);
+if (newNode.canAddMixin(JcrConstants.MIX_VERSIONABLE)) {
+newNode.addMixin(JcrConstants.MIX_VERSIONABLE);
+} else {
+throw new NotExecutableException();
+}
+newNode.getSession().save();
+return newNode;
+}
+
+/**
+ * Checks out the node, sets the property then saves the session and
checks the node back in.
+ * To be used in tests where version history needs to be populated.
+ */
+private void setNodePropertyAndCheckIn(Node node, String propertyName,
String propertyValue) throws Exception {
+node.checkout();
+node.setProperty(propertyName, propertyValue);
+node.getSession().save();
+node.checkin();
+}
+
+/*
+ * 1. Create a versionable unstructured node at
nodeName1/nodeName2/sourceNode
+ * 2. Create a versionable unstructured node at
nodeName1/nodeName3/sourceNode
+ * 3. create version histories for both nodes
+ * 4. remove nodeName1/nodeName3/nodeName1 (that's because move(src,dest)
throws an exception if dest already exists)
+ * 5. move nodeName1/nodeName2/sourceNode to
nodeName1/nodeName3/sourceNode and call session.save()
+ * 6. should work according to JCR specification - reproduces bug for
OAK-8848: will throw ConstraintViolationException
+ * - "Property is protected: jcr:versionHistory
+ */
+@Test
+public void
testMoveNodeWithVersionHistoryOverDeletedNodeWithVersionHistory() throws
Exception {
+
+Session session = getAdminSession();
+Node testRootNode = session.getRootNode().addNode("node1");
+
+String newNodeName = "sourceNode";
+Node sourceParent = testRootNode.addNode("node2"); //
nodeName1/nodeName2
+Node sourceNode = createVersionableNode(sourceParent, newNodeName); //
nodeName1/nodeName2/sourceNode
+Node destParent = testRootNode.addNode("node3"); // nodeName1/nodeName3
+Node destNode = createVersionableNode(destParent, "destNode"); //
nodeName1/nodeName3/sourceNode
+
+String destPath = destNode.getPath();
+
+// add version histories for sourceNode and destNode
+setNodePropertyAndCheckIn(sourceNode,
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on code in PR #1474:
URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644565491
##
oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/MoveVersionableNodeRepositoryTest.java:
##
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.oak.jcr;
+
+import org.apache.jackrabbit.JcrConstants;
+import org.apache.jackrabbit.oak.NodeStoreFixtures;
+import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
+import org.apache.jackrabbit.test.NotExecutableException;
+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.version.Version;
+import javax.jcr.version.VersionHistory;
+import javax.jcr.version.VersionIterator;
+import javax.jcr.version.VersionManager;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static java.util.Collections.singleton;
+import static
org.apache.jackrabbit.oak.commons.FixturesHelper.Fixture.DOCUMENT_NS;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Test for moving versionable nodes over deleted versionable nodes using the
DOCUMENT_NS fixture.
+ */
+public class MoveVersionableNodeRepositoryTest extends AbstractRepositoryTest {
+
+public MoveVersionableNodeRepositoryTest(NodeStoreFixture fixture) {
+super(fixture);
+}
+
+@Parameterized.Parameters(name="{0}")
+public static Collection memoryFixture() {
+return NodeStoreFixtures.asJunitParameters(singleton(DOCUMENT_NS));
+}
+
+/**
+ * Creates a versionable node with the specified name under the given
parent node, then
+ * saves the session.
+ * @param parent
+ * @param nodeName
+ * @return
+ * @throws Exception
+ */
+private Node createVersionableNode(Node parent, String nodeName) throws
Exception {
+Node newNode = (parent.hasNode(nodeName)) ? parent.getNode(nodeName) :
parent.addNode(nodeName);
+if (newNode.canAddMixin(JcrConstants.MIX_VERSIONABLE)) {
+newNode.addMixin(JcrConstants.MIX_VERSIONABLE);
+} else {
+throw new NotExecutableException();
+}
+newNode.getSession().save();
+return newNode;
+}
+
+/**
+ * Checks out the node, sets the property then saves the session and
checks the node back in.
+ * To be used in tests where version history needs to be populated.
+ */
+private void setNodePropertyAndCheckIn(Node node, String propertyName,
String propertyValue) throws Exception {
+node.checkout();
+node.setProperty(propertyName, propertyValue);
+node.getSession().save();
+node.checkin();
+}
+
+/*
+ * 1. Create a versionable unstructured node at
nodeName1/nodeName2/sourceNode
+ * 2. Create a versionable unstructured node at
nodeName1/nodeName3/sourceNode
+ * 3. create version histories for both nodes
+ * 4. remove nodeName1/nodeName3/nodeName1 (that's because move(src,dest)
throws an exception if dest already exists)
+ * 5. move nodeName1/nodeName2/sourceNode to
nodeName1/nodeName3/sourceNode and call session.save()
+ * 6. should work according to JCR specification - reproduces bug for
OAK-8848: will throw ConstraintViolationException
+ * - "Property is protected: jcr:versionHistory
+ */
+@Test
+public void
testMoveNodeWithVersionHistoryOverDeletedNodeWithVersionHistory() throws
Exception {
+
+Session session = getAdminSession();
+Node testRootNode = session.getRootNode().addNode("node1");
+
+String newNodeName = "sourceNode";
+Node sourceParent = testRootNode.addNode("node2"); //
nodeName1/nodeName2
+Node sourceNode = createVersionableNode(sourceParent, newNodeName); //
nodeName1/nodeName2/sourceNode
+Node destParent = testRootNode.addNode("node3"); // nodeName1/nodeName3
+Node destNode = createVersionableNode(destParent, "destNode"); //
nodeName1/nodeName3/sourceNode
+
+String destPath = destNode.getPath();
+
+// add version histories for sourceNode and destNode
+setNodePropertyAndCheckIn(sourceNode,
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644558711
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
Yes, but only then, not always.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644549857
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
"getValue()" will uncompress when the value is compressed, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644545109
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
Not necessarily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644545109
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
Not necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644499232
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -127,8 +188,25 @@ public boolean equals(Object object) {
return true;
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
-return this.name.equals(other.name)
-&& this.value.equals(other.value);
+// Compare names and raw un-parsed values
+if (!this.name.equals(other.name) ||
!this.getValue().equals(other.getValue())) {
Review Comment:
This will always decompress, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644492664
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,225 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644490150
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,225 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644489555
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
Done
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,225 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644489190
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -18,26 +18,45 @@
import java.io.IOException;
import java.io.InputStream;
+import java.io.OutputStream;
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.List;
+import java.util.Objects;
import java.util.Set;
+import org.apache.commons.lang3.RandomStringUtils;
import org.apache.jackrabbit.oak.api.Blob;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.commons.Compression;
import org.apache.jackrabbit.oak.spi.blob.BlobStore;
import org.apache.jackrabbit.oak.spi.blob.MemoryBlobStore;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
+import org.junit.*;
+import org.junit.runners.MethodSorters;
import static org.apache.jackrabbit.guava.common.collect.Lists.newArrayList;
import static org.apache.jackrabbit.guava.common.collect.Sets.newHashSet;
import static org.junit.Assert.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public class DocumentPropertyStateTest {
private static final int BLOB_SIZE = 16 * 1024;
+public static final int COMPRESSED_SIZE = 543;
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644488604
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -38,24 +45,64 @@
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.value.Conversions;
import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* PropertyState implementation with lazy parsing of the JSOP encoded value.
*/
final class DocumentPropertyState implements PropertyState {
+private static final Logger LOG =
LoggerFactory.getLogger(DocumentPropertyState.class);
+
private final DocumentNodeStore store;
private final String name;
private final String value;
private PropertyState parsed;
+private final byte[] compressedValue;
+private final Compression compression;
+
+private static final int DEFAULT_COMPRESSION_THRESHOLD =
SystemPropertySupplier
+.create("oak.documentMK.stringCompressionThreshold ",
-1).loggingTo(LOG).get();
DocumentPropertyState(DocumentNodeStore store, String name, String value) {
+this(store, name, value, Compression.GZIP);
+}
+
+DocumentPropertyState(DocumentNodeStore store, String name, String value,
Compression compression) {
this.store = store;
this.name = name;
-this.value = value;
+if (compression != null && DEFAULT_COMPRESSION_THRESHOLD == -1) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644487143
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644486368
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING),
STRING_HUGEVALUE);
+
+verify(mockCompression,
times(1)).getOutputStream(any(OutputStream.class));
+
+compressionThreshold.set(null, -1);
+
+}
+
+@Test
+public void uncompressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+OutputStream mockOutputStream= mock(OutputStream.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenReturn(mockOutputStream);
+
when(mockCompression.getInputStream(any(InputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", STRING_HUGEVALUE,
mockCompression);
+
+assertEquals(documentPropertyState.getValue(Type.STRING), "{}");
+
+verify(mockCompression,
times(1)).getInputStream(any(InputStream.class));
+
+
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644485936
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -38,24 +45,64 @@
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.value.Conversions;
import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* PropertyState implementation with lazy parsing of the JSOP encoded value.
*/
final class DocumentPropertyState implements PropertyState {
+private static final Logger LOG =
LoggerFactory.getLogger(DocumentPropertyState.class);
+
private final DocumentNodeStore store;
private final String name;
private final String value;
private PropertyState parsed;
+private final byte[] compressedValue;
+private final Compression compression;
+
+private static final int DEFAULT_COMPRESSION_THRESHOLD =
SystemPropertySupplier
+.create("oak.documentMK.stringCompressionThreshold ",
-1).loggingTo(LOG).get();
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GH] (jackrabbit-oak): Workflow run "SonarCloud" is working again!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has succeeded. Run started by GitHub user shodaaan (triggered by stefan-egli). Head commit for run: e9ba9d876e7dc5295a55d5c6d39e059a8c9366be / shodaaan - applied the new full GC mode options in VersionGarbageCollector via the fullGCOptions parameter passed in the constructor - added unit test methods for testing feature toggle and configuration options set in DocumentNodeStoreBuilder, based on existing testing of feature toggles and configuration settings in unit tests - fixed CommitBuilderTest test failures by changing expected exception type Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9549104667 With regards, GitHub Actions via GitBox
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644395621
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -128,7 +188,7 @@ public boolean equals(Object object) {
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
return this.name.equals(other.name)
-&& this.value.equals(other.value);
+&& this.getValue().equals(other.getValue());
Review Comment:
Right, I think there should be distinction between non-compressed,
compressed, mixed cases. Only the compressed one is slightly more involved. For
that one we could use a combination of length comparison, hashcode (calculated
when needed) and/or plain Arrays.equals(byte[],byte[])..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644353388
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -128,7 +188,7 @@ public boolean equals(Object object) {
} else if (object instanceof DocumentPropertyState) {
DocumentPropertyState other = (DocumentPropertyState) object;
return this.name.equals(other.name)
-&& this.value.equals(other.value);
+&& this.getValue().equals(other.getValue());
Review Comment:
We could special-case the compressed case, and compare the byte arrays.
Still expensive.
Maybe we could keep the String's hashCode in order to avoid comparing the
byte arrays in most cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
ionutzpi commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644348052
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
Yes. I did the same idea as Stefan. But Julian insisted to make it
private(it was package-protected). Also, this approach complicated many
tests(trying to solve with reflection)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644336103
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
ah, now I remember. Your comment wasn't visible in this particular review
anymore, so I didn't connect the dots.
I'm actually a fan of facilitating testing, and that can mean to introduce
methods in productive classes that are there solely for testing purpose. That
method can still be non-public but package-protected for example. That in my
view makes things more explicit than using reflection in tests which is
brittle. Moving this method to the test class I assume implies opening up the
actual variable, which seems less favourable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
reschke commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644328794
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -116,7 +163,20 @@ public int count() {
*/
@NotNull
String getValue() {
-return value;
+return value != null ? value : decompress(this.compressedValue);
+}
+
+private String decompress(byte[] value) {
+try {
+return new String(compression.getInputStream(new
ByteArrayInputStream(value)).readAllBytes(), StandardCharsets.UTF_8);
+} catch (IOException e) {
+LOG.error("Failed to decompress property {} value: ", getName(),
e);
+return "\"{}\"";
+}
+}
+
+private byte[] getCompressedValue() {
Review Comment:
It actually was my proposal not to expose it. Maybe we can just *copy* or
*move* it to the test class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] OAK-10803 -- compress/uncompress property [jackrabbit-oak]
stefan-egli commented on code in PR #1526:
URL: https://github.com/apache/jackrabbit-oak/pull/1526#discussion_r1644291222
##
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyState.java:
##
@@ -38,24 +45,64 @@
import org.apache.jackrabbit.oak.plugins.memory.StringPropertyState;
import org.apache.jackrabbit.oak.plugins.value.Conversions;
import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* PropertyState implementation with lazy parsing of the JSOP encoded value.
*/
final class DocumentPropertyState implements PropertyState {
+private static final Logger LOG =
LoggerFactory.getLogger(DocumentPropertyState.class);
+
private final DocumentNodeStore store;
private final String name;
private final String value;
private PropertyState parsed;
+private final byte[] compressedValue;
+private final Compression compression;
+
+private static final int DEFAULT_COMPRESSION_THRESHOLD =
SystemPropertySupplier
+.create("oak.documentMK.stringCompressionThreshold ",
-1).loggingTo(LOG).get();
DocumentPropertyState(DocumentNodeStore store, String name, String value) {
+this(store, name, value, Compression.GZIP);
+}
+
+DocumentPropertyState(DocumentNodeStore store, String name, String value,
Compression compression) {
this.store = store;
this.name = name;
-this.value = value;
+if (compression != null && DEFAULT_COMPRESSION_THRESHOLD == -1) {
Review Comment:
if `compression` is null, then it would go into `else` and run into a NPE
##
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentPropertyStateTest.java:
##
@@ -81,4 +100,179 @@ public void multiValuedBinarySize() throws Exception {
assertEquals(0, reads.size());
}
-}
+@Test
+public void multiValuedAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+List blobs = newArrayList();
+for (int i = 0; i < 13; i++) {
+blobs.add(builder.createBlob(new RandomStream(BLOB_SIZE, i)));
+}
+builder.child(TEST_NODE).setProperty("p", blobs, Type.BINARIES);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.BINARIES, Objects.requireNonNull(p).getType());
+assertEquals(13, p.count());
+
+reads.clear();
+assertEquals(BLOB_SIZE, p.size(0));
+// must not read the blob via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringBelowThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", "dummy", Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(5, p.size(0));
+// must not read the string via stream
+assertEquals(0, reads.size());
+}
+
+@Test
+public void stringAboveThresholdSize() throws Exception {
+NodeBuilder builder = ns.getRoot().builder();
+builder.child(TEST_NODE).setProperty("p", STRING_HUGEVALUE,
Type.STRING);
+TestUtils.merge(ns, builder);
+
+PropertyState p =
ns.getRoot().getChildNode(TEST_NODE).getProperty("p");
+assertEquals(Type.STRING, Objects.requireNonNull(p).getType());
+assertEquals(1, p.count());
+
+reads.clear();
+assertEquals(10050, p.size(0));
+// must not read the string via streams
+assertEquals(0, reads.size());
+}
+
+@Test
+public void compressValueThrowsException() throws IOException,
NoSuchFieldException, IllegalAccessException {
+DocumentNodeStore mockDocumentStore = mock(DocumentNodeStore.class);
+Compression mockCompression = mock(Compression.class);
+
when(mockCompression.getOutputStream(any(OutputStream.class))).thenThrow(new
IOException("Compression failed"));
+
+Field compressionThreshold =
DocumentPropertyState.class.getDeclaredField("DEFAULT_COMPRESSION_THRESHOLD");
+compressionThreshold.setAccessible(true);
+Field modifiersField = Field.class.getDeclaredField("modifiers");
+modifiersField.setAccessible(true);
+modifiersField.setInt(compressionThreshold,
compressionThreshold.getModifiers() & ~Modifier.FINAL);
+
+compressionThreshold.set(null, DEFAULT_COMPRESSION_THRESHOLD);
+
+DocumentPropertyState documentPropertyState = new
DocumentPropertyState(mockDocumentStore, "p", "\"" + STRING_HUGEVALUE + "\"",
mockCompression);
+
+
[GH] (jackrabbit-oak): Workflow run "SonarCloud" failed!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has failed. Run started by GitHub user dulceanu (triggered by dulceanu). Head commit for run: 7c774d7b66d767e52acd61121c0f0649fc958804 / Andrei Dulceanu OAK-10898 - Allow AzureCheck and AzureCompact to be built directly with a CloudBlobDirectory (#1540) Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9563486306 With regards, GitHub Actions via GitBox
[GH] (jackrabbit-oak): Workflow run "SonarCloud" is working again!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has succeeded. Run started by GitHub user t-rana (triggered by smiroslav). Head commit for run: 05794604ceaa68534f1e4663148cb4ee4e249d4e / t-rana add service principal support while deleting container in oak-run-commons Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9563659968 With regards, GitHub Actions via GitBox
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
shodaaan commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644246364 ## RELEASE-NOTES.txt: ## @@ -1,4 +1,4 @@ -Release Notes -- Apache Jackrabbit Oak -- Version 1.64.0 +Release Notes -- Apache Jackrabbit Oak -- Version 1.62.0 Review Comment: Fixed it, thank you for the observation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] add service principal support while deleting container in oak-run-commons [jackrabbit-oak]
t-rana opened a new pull request, #1542: URL: https://github.com/apache/jackrabbit-oak/pull/1542 …mons -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GH] (jackrabbit-oak): Workflow run "SonarCloud" failed!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has failed. Run started by GitHub user stefan-egli (triggered by stefan-egli). Head commit for run: 28ca094a749ec72ac6005a6b7eed6b77b9392a25 / ionutzpi OAK-10890 - Added log warning (#1535) * OAK-10890 - Added log warning * OAK-10890 - Added prefix to log warning - Co-authored-by: pirlogea Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9562806922 With regards, GitHub Actions via GitBox
[GH] (jackrabbit-oak): Workflow run "SonarCloud" failed!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has failed. Run started by GitHub user nfsantos (triggered by nfsantos). Head commit for run: 4115001cffd1aefecb38221a478bcd59cc5f3065 / Nuno Santos OAK-10894 - DocumentNodeStore: expose readNode as package private (#1533) Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9562724346 With regards, GitHub Actions via GitBox
Re: [PR] OAK-10898 - Allow AzureCheck and AzureCompact to be built directly wi… [jackrabbit-oak]
dulceanu merged PR #1540: URL: https://github.com/apache/jackrabbit-oak/pull/1540 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Issue/oak 8848 [jackrabbit-oak]
stefan-egli commented on code in PR #1474: URL: https://github.com/apache/jackrabbit-oak/pull/1474#discussion_r1644227441 ## RELEASE-NOTES.txt: ## @@ -1,4 +1,4 @@ -Release Notes -- Apache Jackrabbit Oak -- Version 1.64.0 +Release Notes -- Apache Jackrabbit Oak -- Version 1.62.0 Review Comment: this file shouldn't be part of the PR -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] OAK-10867: add service principal support in oak-run commons [jackrabbit-oak]
t-rana commented on PR #1513: URL: https://github.com/apache/jackrabbit-oak/pull/1513#issuecomment-2175731261 Closing this PR for now, will raise a new one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] OAK-10867: add service principal support in oak-run commons [jackrabbit-oak]
t-rana closed pull request #1513: OAK-10867: add service principal support in oak-run commons URL: https://github.com/apache/jackrabbit-oak/pull/1513 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GH] (jackrabbit-oak): Workflow run "SonarCloud" failed!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has failed. Run started by GitHub user amit-jain (triggered by amit-jain). Head commit for run: 868e308a1d7832f086d397d528b2b786bd6e46cf / Tushar <145645280+t-r...@users.noreply.github.com> Issues/oak 10781 (#1518) OAK-10781: Access Token refresh in oak-blob-cloud-azure * only one access token will exist per class instance, and there will only be one token refresh exectuor that will check and refresh the access token. * close executor * reduce token refresh delay to 1 minute Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9562356616 With regards, GitHub Actions via GitBox
[GH] (jackrabbit-oak): Workflow run "SonarCloud" is working again!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has succeeded. Run started by GitHub user nfsantos (triggered by nfsantos). Head commit for run: 475cc6dc2aa01430339025186d2c27e2412b7128 / Nuno Santos Bypass the DocumentNodeState cache when resolving paths downloaded from Mongo. Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9562929168 With regards, GitHub Actions via GitBox
Re: [PR] OAK-10890 - Added log warning [jackrabbit-oak]
stefan-egli merged PR #1535: URL: https://github.com/apache/jackrabbit-oak/pull/1535 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] OAK-10901 - Bypass the DocumentNodeState cache when resolving paths downloaded from Mongo. [jackrabbit-oak]
nfsantos opened a new pull request, #1541: URL: https://github.com/apache/jackrabbit-oak/pull/1541 (no comment) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GH] (jackrabbit-oak): Workflow run "SonarCloud" failed!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has failed. Run started by GitHub user dulceanu (triggered by dulceanu). Head commit for run: eb73debd5539f0cccfe6541fb674b1af74d7cb08 / Andrei Dulceanu OAK-10898 - Allow AzureCheck and AzureCompact to be built directly with a CloudBlobDirectory Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9562177966 With regards, GitHub Actions via GitBox
Re: [PR] OAK-10894 - DocumentNodeStore: expose readNode as package private [jackrabbit-oak]
nfsantos merged PR #1533: URL: https://github.com/apache/jackrabbit-oak/pull/1533 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Issues/oak 10781 [jackrabbit-oak]
amit-jain merged PR #1518: URL: https://github.com/apache/jackrabbit-oak/pull/1518 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] OAK-10898 - Allow AzureCheck and AzureCompact to be built directly wi… [jackrabbit-oak]
dulceanu opened a new pull request, #1540: URL: https://github.com/apache/jackrabbit-oak/pull/1540 …th a CloudBlobDirectory -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Issues/oak 10781 [jackrabbit-oak]
nit0906 commented on code in PR #1518:
URL: https://github.com/apache/jackrabbit-oak/pull/1518#discussion_r1644093746
##
oak-blob-cloud-azure/src/main/java/org/apache/jackrabbit/oak/blob/cloud/azure/blobstorage/AzureBlobContainerProvider.java:
##
@@ -205,19 +212,33 @@ private CloudBlobContainer
getBlobContainerFromServicePrincipals(@Nullable BlobR
@NotNull
private StorageCredentialsToken getStorageCredentials() {
-ClientSecretCredential clientSecretCredential = new
ClientSecretCredentialBuilder()
-.clientId(clientId)
-.clientSecret(clientSecret)
-.tenantId(tenantId)
-.build();
-AccessToken accessToken = clientSecretCredential.getTokenSync(new
TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE));
-if (accessToken == null ||
StringUtils.isBlank(accessToken.getToken())) {
-log.error("Access token is null or empty");
-throw new IllegalArgumentException("Could not connect to azure
storage, access token is null or empty");
+boolean isAccessTokenGenerated = false;
+/* generate access token, the same token will be used for subsequent
access
+ * generated token is valid for 1 hour only and will be refreshed in
background
+ * */
+if (accessToken == null) {
+clientSecretCredential = new ClientSecretCredentialBuilder()
+.clientId(clientId)
+.clientSecret(clientSecret)
+.tenantId(tenantId)
+.build();
+accessToken = clientSecretCredential.getTokenSync(new
TokenRequestContext().addScopes(AZURE_DEFAULT_SCOPE));
+if (accessToken == null ||
StringUtils.isBlank(accessToken.getToken())) {
+log.error("Access token is null or empty");
+throw new IllegalArgumentException("Could not connect to azure
storage, access token is null or empty");
+}
+storageCredentialsToken = new StorageCredentialsToken(accountName,
accessToken.getToken());
+isAccessTokenGenerated = true;
+}
+
+Objects.requireNonNull(storageCredentialsToken, "storage credentials
token cannot be null");
+
+// start refresh token executor only when the access token is first
generated
+if (isAccessTokenGenerated) {
+log.info("starting refresh token task at: {}",
OffsetDateTime.now());
+TokenRefresher tokenRefresher = new TokenRefresher();
+executorService.scheduleWithFixedDelay(tokenRefresher,
TOKEN_REFRESHER_INITIAL_DELAY, TOKEN_REFRESHER_DELAY, TimeUnit.MINUTES);
Review Comment:
Can you add a comment here describing why the 1 minute delay is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GH] (jackrabbit-oak): Workflow run "SonarCloud" failed!
The GitHub Actions job "SonarCloud" on jackrabbit-oak.git has failed. Run started by GitHub user nfsantos (triggered by nfsantos). Head commit for run: 7bee7ce77913d8149688f14c8fc1c9cdfbea0f1d / Nuno Santos OAK-10899 - Do not remove field with size estimation from NodeDocuments downloaded from Mongo (#1530) Report URL: https://github.com/apache/jackrabbit-oak/actions/runs/9560873812 With regards, GitHub Actions via GitBox
Re: [PR] OAK-10889 - Indexing job: do not remove field with size estimation from NodeDocuments downloaded from Mongo [jackrabbit-oak]
nfsantos merged PR #1530: URL: https://github.com/apache/jackrabbit-oak/pull/1530 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
