mreutegg commented on code in PR #863:
URL: https://github.com/apache/jackrabbit-oak/pull/863#discussion_r1132558964
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1487,8 +1499,23 @@ public <T extends Document> boolean create(Collection<T>
collection, List<Update
}
insertSuccess = true;
return true;
- } catch (MongoException e) {
- return false;
+ } catch (BsonMaximumSizeExceededException e) {
Review Comment:
The new tests do not seem to trigger this exception.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1078,23 +1090,25 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
}
- int size = updateOp.toString().length();
- if(size > SIZE_LIMIT) {
- LOG.warn("Document with ID={} has size={} that exceeds 16MB
size limit", updateOp.getId(), size);
- }
// conditional update failed or not possible
// perform operation and get complete document
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 (MongoWriteException e) {
+ WriteError error = e.getError();
Review Comment:
`error` is not used.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1078,23 +1090,25 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
}
- int size = updateOp.toString().length();
- if(size > SIZE_LIMIT) {
- LOG.warn("Document with ID={} has size={} that exceeds 16MB
size limit", updateOp.getId(), size);
- }
// conditional update failed or not possible
// perform operation and get complete document
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 (MongoWriteException e) {
Review Comment:
It seems to me the new tests do not exercise this code path with the
exception.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1059,13 +1064,20 @@ 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) {
Review Comment:
It seems to me the new tests do not exercise this code path with the
exception.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1059,13 +1064,20 @@ 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 error = e.getError();
Review Comment:
`error` is not used.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1078,23 +1090,25 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
}
- int size = updateOp.toString().length();
- if(size > SIZE_LIMIT) {
- LOG.warn("Document with ID={} has size={} that exceeds 16MB
size limit", updateOp.getId(), size);
- }
// conditional update failed or not possible
// perform operation and get complete document
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 (MongoWriteException e) {
+ WriteError error = e.getError();
+ LOG.warn(e.getMessage(), updateOp.getId(),
updateOp.getConditions(), oldNode.size());
Review Comment:
This looks strange. Does the message format really have three placeholders
for id, conditions and size? Why log conditions?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1123,7 +1137,13 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
}
return oldDoc;
- } catch (Exception e) {
+ } catch (MongoCommandException e) {
+ LOG.warn(e.getMessage(), updateOp.getId(),
updateOp.getConditions());
+ throw handleException(e, collection, updateOp.getId());
+ } catch (BsonMaximumSizeExceededException e){
+ LOG.warn(e.getMessage(), updateOp.getId());
+ throw handleException(e, collection, updateOp.getId());
Review Comment:
New tests do not seem to exercise this code/exception. Also, I'm not sure
the log message is formatted properly to take the id as argument.
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDBExceptionTest.java:
##########
@@ -136,32 +138,118 @@ public void idInExceptionMessage() {
}
@Test
- @Ignore
- public void add16MBDoc() throws Exception {
- //DocumentStore docStore = openDocumentStore();
+ public void createOrUpdate16MBDoc() {
- UpdateOp updateOp = new UpdateOp("/", false);
+ UpdateOp updateOp = new UpdateOp("/", true);
+ updateOp = create16MBProp(updateOp);
- // create a 1 MB property
- char[] chars = new char[1024 * 1024];
+ exceptionMsg = "Document to upsert is larger than 1677721";
+ setExceptionMsg();
+ try {
+ store.createOrUpdate(Collection.NODES, updateOp);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ }
+ }
- Arrays.fill(chars, '0');
- String content = new String(chars);
+ @Test
+ public void multiCreateOrUpdate16MBDoc() {
+
+ List<UpdateOp> updateOps = new ArrayList<UpdateOp>();
+ UpdateOp updateOp = new UpdateOp("/", true);
+ updateOp = create1MBProp(updateOp);
+ UpdateOp updateOp1 = new UpdateOp("/", true);
+ updateOp1 = create1MBProp(updateOp1);
Review Comment:
This is a rather unusual combination of update operation. Both target the
same document id. Is this intentional?
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDBExceptionTest.java:
##########
@@ -136,32 +138,118 @@ public void idInExceptionMessage() {
}
@Test
- @Ignore
- public void add16MBDoc() throws Exception {
- //DocumentStore docStore = openDocumentStore();
+ public void createOrUpdate16MBDoc() {
- UpdateOp updateOp = new UpdateOp("/", false);
+ UpdateOp updateOp = new UpdateOp("/", true);
+ updateOp = create16MBProp(updateOp);
- // create a 1 MB property
- char[] chars = new char[1024 * 1024];
+ exceptionMsg = "Document to upsert is larger than 1677721";
+ setExceptionMsg();
+ try {
+ store.createOrUpdate(Collection.NODES, updateOp);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
Review Comment:
I'd rather be interested to see the exception message contains the id of the
document.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1123,7 +1137,13 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
}
return oldDoc;
- } catch (Exception e) {
+ } catch (MongoCommandException e) {
+ LOG.warn(e.getMessage(), updateOp.getId(),
updateOp.getConditions());
Review Comment:
Same as above. This looks strange to me.
##########
oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDBExceptionTest.java:
##########
@@ -136,32 +138,118 @@ public void idInExceptionMessage() {
}
@Test
- @Ignore
- public void add16MBDoc() throws Exception {
- //DocumentStore docStore = openDocumentStore();
+ public void createOrUpdate16MBDoc() {
- UpdateOp updateOp = new UpdateOp("/", false);
+ UpdateOp updateOp = new UpdateOp("/", true);
+ updateOp = create16MBProp(updateOp);
- // create a 1 MB property
- char[] chars = new char[1024 * 1024];
+ exceptionMsg = "Document to upsert is larger than 1677721";
+ setExceptionMsg();
+ try {
+ store.createOrUpdate(Collection.NODES, updateOp);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
+ }
+ }
- Arrays.fill(chars, '0');
- String content = new String(chars);
+ @Test
+ public void multiCreateOrUpdate16MBDoc() {
+
+ List<UpdateOp> updateOps = new ArrayList<UpdateOp>();
+ UpdateOp updateOp = new UpdateOp("/", true);
+ updateOp = create1MBProp(updateOp);
+ UpdateOp updateOp1 = new UpdateOp("/", true);
+ updateOp1 = create1MBProp(updateOp1);
+
+ updateOps.add(updateOp);
+ updateOps.add(updateOp1);
+
+ store.createOrUpdate(Collection.NODES, updateOps);
+ updateOp1 = create16MBProp(updateOp1);
+ updateOps.add(updateOp1);
- //create more than 16MB properties
- for (int i = 0; i < 17; i++) {
- updateOp.set("property"+ Integer.toString(i), content);
+ exceptionMsg = "Resulting document after update is larger than
16777216";
+ setExceptionMsg();
+
+ try {
+ store.createOrUpdate(Collection.NODES, updateOps);
+ fail("DocumentStoreException expected");
+ } catch (DocumentStoreException e) {
+ assertThat(e.getMessage(), containsString(exceptionMsg));
Review Comment:
Again, it would be better to check if the exception mentions the document id.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1031,6 +1035,7 @@ private <T extends Document> T
findAndModify(Collection<T> collection,
}
final Stopwatch watch = startWatch();
boolean newEntry = false;
+ Throwable t = null;
Review Comment:
Throwable t is never used.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java:
##########
@@ -1487,8 +1499,23 @@ 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) {
+ if (doc.toString().length() > SIZE_LIMIT) {
Review Comment:
IIUC `doc.toString()` simply returns the _id value as a String. I don't
think that's the intention here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
