You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "mreutegg (via GitHub)" <gi...@apache.org> on 2023/04/06 13:14:20 UTC
[GitHub] [jackrabbit-oak] mreutegg commented on a diff in pull request #863: OAK-10127 - Log warn message when MongoDB document is big
mreutegg commented on code in PR #863:
URL: https://github.com/apache/jackrabbit-oak/pull/863#discussion_r1157164980
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDBExceptionTest.java:
##########
@@ -132,8 +137,129 @@ public void idInExceptionMessage() {
}
}
+ @Test
+ public void createOrUpdate16MBDoc() {
+
+ UpdateOp updateOp = new UpdateOp("/foo", true);
+ updateOp = create16MBProp(updateOp);
+ exceptionMsg = "Document to upsert is larger than 16777216";
+ try {
+ store.createOrUpdate(Collection.NODES, updateOp);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ }
+ }
+
+ @Test
+ public void update16MBDoc() {
+
+ String docName = "/foo";
+ UpdateOp updateOp = new UpdateOp(docName, true);
+ updateOp = create1MBProp(updateOp);
+ store.createOrUpdate(Collection.NODES, updateOp);
+ updateOp = create16MBProp(updateOp);
+ exceptionMsg = "Resulting document after update is larger than 16777216";
+ try {
+ store.createOrUpdate(Collection.NODES, updateOp);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ assertThat(e.getMessage(), containsString(docName));
+ }
+ }
+
+ @Test
+ public void multiCreateOrUpdate16MBDoc() {
+
+ List<UpdateOp> updateOps = new ArrayList<UpdateOp>();
+
+ UpdateOp op = new UpdateOp("/test", true);
+ op = create1MBProp(op);
+
+ store.createOrUpdate(Collection.NODES, op);
+
+ UpdateOp op1 = new UpdateOp("/foo", true);
+ op1 = create16MBProp(op1);
+
+ // Updating both doc with 16MB
+ op = create16MBProp(op);
+ updateOps.add(op);
+ updateOps.add(op1);
+ exceptionMsg = "Resulting document after update is larger than 16777216";
+
+ try {
+ store.createOrUpdate(Collection.NODES, updateOps);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ }
+ }
+
+ @Test
+ public void create16MBDoc() {
+
+ List<UpdateOp> updateOps = new ArrayList<UpdateOp>();
+
+ UpdateOp op1 = new UpdateOp("/test", true);
+ op1 = create1MBProp(op1);
+
+ UpdateOp op2 = new UpdateOp("/foo", false);
+ op2 = create1MBProp(op2);
+ op2 = create16MBProp(op2);
+
+ updateOps.add(op1);
+ updateOps.add(op2);
+ String id = op2.getId();
+ exceptionMsg = "Payload document size is larger than maximum of 16777216.";
+ try {
+ store.create(Collection.NODES, updateOps);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ }
+ }
+
+ @Test
+ public void findAndUpdate16MBDoc() throws Exception {
+ UpdateOp op = new UpdateOp("/foo", true);
+ op = create1MBProp(op);
+ store.createOrUpdate(Collection.NODES, op);
+ op = create16MBProp(op);
+ exceptionMsg = "Resulting document after update is larger than 16777216";
+ try {
+ store.findAndUpdate(Collection.NODES, op);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ }
+
+ }
+
private void setExceptionMsg() {
client.setExceptionBeforeUpdate(exceptionMsg);
client.setExceptionBeforeQuery(exceptionMsg);
}
+
+ private UpdateOp create1MBProp(UpdateOp op) {
+ // create a 1 MB property
+ char[] chars = new char[1024 * 1024];
+ Arrays.fill(chars, '0');
+ String content = new String(chars);
+ op.set("property0", content);
+ return op;
+ }
+ private UpdateOp create16MBProp(UpdateOp op) {
+ // create a 1 MB property
+ char[] chars = new char[1024 * 1024];
+ Arrays.fill(chars, '0');
+ String content = new String(chars);
+ op.set("property0", content);
+
+ //create 16MB property
+ for (int i = 1; i < 16; i++) {
+ op.set("property"+ Integer.toString(i), content);
Review Comment:
Integer.toString(i) is unnecessary.
```suggestion
op.set("property"+ i, content);
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1079,14 +1095,22 @@ private <T extends Document> T findAndModify(Collection<T> collection,
Bson query = createQueryForUpdate(updateOp.getId(), updateOp.getConditions());
FindOneAndUpdateOptions options = new FindOneAndUpdateOptions()
.returnDocument(ReturnDocument.BEFORE).upsert(upsert);
- BasicDBObject oldNode = execute(session -> {
- if (session != null) {
- return dbCollection.findOneAndUpdate(session, query, update, options);
- } else {
- return dbCollection.findOneAndUpdate(query, update, options);
- }
- }, collection);
-
+ BasicDBObject oldNode = null;
+ try {
+ oldNode = execute(session -> {
+ if (session != null) {
+ return dbCollection.findOneAndUpdate(session, query, update, options);
+ } else {
+ return dbCollection.findOneAndUpdate(query, update, options);
+ }
+ }, collection);
+ } catch (MongoCommandException e) {
+ LOG.error("Failed to update the document with Id={} has size={} " +
+ "with error message '{}'",
+ updateOp.getId(), updateOp.toString().length(),
+ SIZE_LIMIT, e.getMessage());
+ throw handleException(e, collection, updateOp.getId());
Review Comment:
Same as above.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1470,8 +1495,16 @@ public <T extends Document> boolean create(Collection<T> collection, List<Update
}
insertSuccess = true;
return true;
- } catch (MongoException e) {
- return false;
+ } catch (BsonMaximumSizeExceededException e) {
+ for (T doc : docs) {
+ // doc.getMemory()/2 - converting from UTF-16 to UTF-8
+ if (doc.getMemory()/2 > SIZE_LIMIT) {
+ LOG.error("Failed to create the document with Id={} has size={}" +
Review Comment:
There are three placeholders, but four arguments.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1055,13 +1063,21 @@ private <T extends Document> T findAndModify(Collection<T> collection,
Filters.eq(Document.MOD_COUNT, modCount)
);
- UpdateResult result = execute(session -> {
- if (session != null) {
- return dbCollection.updateOne(session, query, update);
- } else {
- return dbCollection.updateOne(query, update);
- }
- }, collection);
+ UpdateResult result = null;
+ try {
+ result = execute(session -> {
+ if (session != null) {
+ return dbCollection.updateOne(session, query, update);
+ } else {
+ return dbCollection.updateOne(query, update);
+ }
+ }, collection);
+ } catch (MongoWriteException e) {
+ WriteError werr = e.getError();
+ LOG.error("Failed to update the document with Id={} and size={} with error message '{}'",
+ updateOp.getId(), updateOp.toString().length(), werr.getMessage());
+ throw handleException(e, collection, updateOp.getId());
Review Comment:
I think this exception handling should go to the end of the already existing try/catch block. With this change, the exception is caught, re-thrown, caught and re-thrown again.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1470,8 +1495,16 @@ public <T extends Document> boolean create(Collection<T> collection, List<Update
}
insertSuccess = true;
return true;
- } catch (MongoException e) {
- return false;
+ } catch (BsonMaximumSizeExceededException e) {
+ for (T doc : docs) {
+ // doc.getMemory()/2 - converting from UTF-16 to UTF-8
+ if (doc.getMemory()/2 > SIZE_LIMIT) {
+ LOG.error("Failed to create the document with Id={} has size={}" +
+ " with error message '{}",
+ doc.getId(), doc.getMemory()/2, SIZE_LIMIT, e.getMessage());
+ }
+ }
+ throw handleException(e, collection, Document.ID);
Review Comment:
The third parameter is not meaningful. It should be the actual ID values.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1055,13 +1063,21 @@ private <T extends Document> T findAndModify(Collection<T> collection,
Filters.eq(Document.MOD_COUNT, modCount)
);
- UpdateResult result = execute(session -> {
- if (session != null) {
- return dbCollection.updateOne(session, query, update);
- } else {
- return dbCollection.updateOne(query, update);
- }
- }, collection);
+ UpdateResult result = null;
+ try {
+ result = execute(session -> {
+ if (session != null) {
+ return dbCollection.updateOne(session, query, update);
+ } else {
+ return dbCollection.updateOne(query, update);
+ }
+ }, collection);
+ } catch (MongoWriteException e) {
+ WriteError werr = e.getError();
+ LOG.error("Failed to update the document with Id={} and size={} with error message '{}'",
+ updateOp.getId(), updateOp.toString().length(), werr.getMessage());
Review Comment:
Do we really need the size of the UpdateOp? If yes, then I think it would be better to let the UpdateOp calculate it instead of formatting the UpdateOp as a String and getting the number of characters. Converting to String will use quite a bit of memory when the UpdateOp is big.
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDBExceptionTest.java:
##########
@@ -30,6 +30,11 @@
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
+import org.junit.Ignore;
Review Comment:
Unused.
```suggestion
```
--
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: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org