You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "rishabhdaim (via GitHub)" <gi...@apache.org> on 2023/04/06 08:01:15 UTC
[GitHub] [jackrabbit-oak] rishabhdaim commented on a diff in pull request #863: OAK-10127 - Log warn message when MongoDB document is big
rishabhdaim commented on code in PR #863:
URL: https://github.com/apache/jackrabbit-oak/pull/863#discussion_r1159409770
##########
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());
Review Comment:
You have provided 4 arguments whereas you only have 3 placeholders.
##########
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>();
Review Comment:
```suggestion
List<UpdateOp> updateOps = new ArrayList<>();
```
##########
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());
Review Comment:
Same as above. 4 arguments but only 3 placeholders.
##########
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);
Review Comment:
This code can be extracted out to a common method to create a content worth 1MB
##########
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>();
Review Comment:
```suggestion
List<UpdateOp> updateOps = new ArrayList<>();
```
--
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