This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 0b05fa355e1d9c85e0f16f3ecfd241f7dbb823e3 Author: Benoit Tellier <[email protected]> AuthorDate: Thu Oct 22 11:42:48 2020 +0700 JAMES-3277 Optimization: JMAP Rely on MessageManager::moveMessage (Draft) We notice that many slow traces at the JMAP level are message moves updates, generating a high count of Cassandra queries. Query count before: 22m+12 - 6 message => 146 queries - 9 messages => 210 queries - 12 messages => 276 queries Query count after: 16m+15 - 6 message => 111 queries - 9 messages => 159 queries - 12 messages => 207 queries --- .../org/apache/james/mailbox/MailboxManager.java | 2 + .../james/mailbox/store/StoreMailboxManager.java | 8 +++ .../methods/integration/SetMessagesMethodTest.java | 74 ++++++++++++++++++++++ .../draft/methods/SetMessagesUpdateProcessor.java | 60 ++++++++++++++++++ .../james/jmap/draft/model/UpdateMessagePatch.java | 6 ++ 5 files changed, 150 insertions(+) diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java b/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java index 01740b8..a175127 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java @@ -289,6 +289,8 @@ public interface MailboxManager extends RequestAware, RightManager, MailboxAnnot */ List<MessageRange> moveMessages(MessageRange set, MailboxPath from, MailboxPath to, MailboxSession session) throws MailboxException; + List<MessageRange> moveMessages(MessageRange set, MailboxId from, MailboxId to, MailboxSession session) throws MailboxException; + enum MailboxSearchFetchType { Minimal, Counters diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java index 2ca6eb0..97c1972 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java @@ -623,6 +623,14 @@ public class StoreMailboxManager implements MailboxManager { } @Override + public List<MessageRange> moveMessages(MessageRange set, MailboxId from, MailboxId to, MailboxSession session) throws MailboxException { + StoreMessageManager toMailbox = (StoreMessageManager) getMailbox(to, session); + StoreMessageManager fromMailbox = (StoreMessageManager) getMailbox(from, session); + + return configuration.getMoveBatcher().batchMessages(set, messageRange -> fromMailbox.moveTo(messageRange, toMailbox, session)); + } + + @Override public Flux<MailboxMetaData> search(MailboxQuery expression, MailboxSearchFetchType fetchType, MailboxSession session) { Mono<List<Mailbox>> mailboxesMono = searchMailboxes(expression, session, Right.Lookup).collectList(); diff --git a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMessagesMethodTest.java b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMessagesMethodTest.java index 2fbed4fff..fc5d16a 100644 --- a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMessagesMethodTest.java +++ b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMessagesMethodTest.java @@ -1130,6 +1130,80 @@ public abstract class SetMessagesMethodTest { .body(FIRST_MAILBOX + ".unreadMessages", equalTo(0)); } + @Test + public void massiveMessageMoveShouldBeApplied() throws MailboxException { + mailboxProbe.createMailbox(MailboxConstants.USER_NAMESPACE, USERNAME.asString(), "mailbox"); + mailboxProbe.createMailbox(MailboxConstants.USER_NAMESPACE, USERNAME.asString(), DefaultMailboxes.DRAFTS); + mailboxProbe.createMailbox(MailboxConstants.USER_NAMESPACE, USERNAME.asString(), DefaultMailboxes.OUTBOX); + mailboxProbe.createMailbox(MailboxConstants.USER_NAMESPACE, USERNAME.asString(), DefaultMailboxes.SENT); + MailboxId mailboxId = mailboxProbe.createMailbox(MailboxConstants.USER_NAMESPACE, USERNAME.asString(), DefaultMailboxes.TRASH); + mailboxProbe.createMailbox(MailboxConstants.USER_NAMESPACE, USERNAME.asString(), DefaultMailboxes.SPAM); + + ComposedMessageId message1 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message2 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message3 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags(Flags.Flag.SEEN)); + ComposedMessageId message4 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message5 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags(Flags.Flag.ANSWERED)); + ComposedMessageId message6 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message7 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message8 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message9 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags(Flags.Flag.SEEN)); + ComposedMessageId message10 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + ComposedMessageId message11 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags(Flags.Flag.ANSWERED)); + ComposedMessageId message12 = mailboxProbe.appendMessage(USERNAME.asString(), USER_MAILBOX, + new ByteArrayInputStream("Subject: test\r\n\r\ntestmail".getBytes(StandardCharsets.UTF_8)), new Date(), false, new Flags()); + + String serializedMessageId1 = message1.getMessageId().serialize(); + String serializedMessageId2 = message2.getMessageId().serialize(); + String serializedMessageId3 = message3.getMessageId().serialize(); + String serializedMessageId4 = message4.getMessageId().serialize(); + String serializedMessageId5 = message5.getMessageId().serialize(); + String serializedMessageId6 = message6.getMessageId().serialize(); + String serializedMessageId7 = message7.getMessageId().serialize(); + String serializedMessageId8 = message8.getMessageId().serialize(); + String serializedMessageId9 = message9.getMessageId().serialize(); + String serializedMessageId10 = message10.getMessageId().serialize(); + String serializedMessageId11 = message11.getMessageId().serialize(); + String serializedMessageId12 = message12.getMessageId().serialize(); + + // When + given() + .header("Authorization", accessToken.asString()) + .body(String.format("[[\"setMessages\", {\"update\": {" + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]}, " + + " \"%s\" : { \"mailboxIds\": [" + mailboxId.serialize() + "]} " + + "} }, \"#0\"]]", serializedMessageId1, serializedMessageId2, serializedMessageId3, + serializedMessageId4, serializedMessageId5, serializedMessageId6, + serializedMessageId7, serializedMessageId8, serializedMessageId9, + serializedMessageId10, serializedMessageId11, serializedMessageId12)) + .when() + .post("/jmap") + // Then + .then() + .log().ifValidationFails().body(ARGUMENTS + ".updated", hasSize(12)); + } + @Category(BasicFeature.class) @Test public void sendingAMailShouldLeadToAppropriateMailboxCountersOnSent() { diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMessagesUpdateProcessor.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMessagesUpdateProcessor.java index 84d31e5..9f4c176 100644 --- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMessagesUpdateProcessor.java +++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMessagesUpdateProcessor.java @@ -153,6 +153,8 @@ public class SetMessagesUpdateProcessor implements SetMessagesProcessor { if (isAMassiveFlagUpdate(patches, messages)) { applyRangedFlagUpdate(patches, messages, responseBuilder, mailboxSession); + } else if (isAMassiveMove(patches, messages)) { + applyMove(patches, messages, responseBuilder, mailboxSession); } else { patches.forEach((id, patch) -> { if (patch.isValid()) { @@ -172,6 +174,14 @@ public class SetMessagesUpdateProcessor implements SetMessagesProcessor { && messages.size() > 3; } + private boolean isAMassiveMove(Map<MessageId, UpdateMessagePatch> patches, Multimap<MessageId, ComposedMessageIdWithMetaData> messages) { + // The same patch, that only represents a flag update, is applied to messages within a single mailbox + return StreamUtils.isSingleValued(patches.values().stream()) + && StreamUtils.isSingleValued(messages.values().stream().map(metaData -> metaData.getComposedMessageId().getMailboxId())) + && patches.values().iterator().next().isOnlyAMove() + && messages.size() > 3; + } + private void applyRangedFlagUpdate(Map<MessageId, UpdateMessagePatch> patches, Multimap<MessageId, ComposedMessageIdWithMetaData> messages, SetMessagesResponse.Builder responseBuilder, MailboxSession mailboxSession) { MailboxId mailboxId = messages.values() .iterator() @@ -198,6 +208,56 @@ public class SetMessagesUpdateProcessor implements SetMessagesProcessor { } catch (MailboxException e) { messageIds .forEach(messageId -> handleMessageUpdateException(messageId, responseBuilder, e)); + } catch (IllegalArgumentException e) { + ValidationResult invalidPropertyKeywords = ValidationResult.builder() + .property(MessageProperties.MessageProperty.keywords.asFieldName()) + .message(e.getMessage()) + .build(); + + messageIds + .forEach(messageId -> handleInvalidRequest(responseBuilder, messageId, ImmutableList.of(invalidPropertyKeywords), patch)); + } + }); + } else { + messages.keySet() + .forEach(messageId -> handleInvalidRequest(responseBuilder, messageId, patch.getValidationErrors(), patch)); + } + } + + private void applyMove(Map<MessageId, UpdateMessagePatch> patches, Multimap<MessageId, ComposedMessageIdWithMetaData> messages, SetMessagesResponse.Builder responseBuilder, MailboxSession mailboxSession) { + MailboxId mailboxId = messages.values() + .iterator() + .next() + .getComposedMessageId() + .getMailboxId(); + UpdateMessagePatch patch = patches.values().iterator().next(); + List<MessageRange> uidRanges = MessageRange.toRanges(messages.values().stream().map(metaData -> metaData.getComposedMessageId().getUid()) + .distinct() + .collect(Guavate.toImmutableList())); + + if (patch.isValid()) { + uidRanges.forEach(range -> { + ImmutableList<MessageId> messageIds = messages.entries() + .stream() + .filter(entry -> range.includes(entry.getValue().getComposedMessageId().getUid())) + .map(Map.Entry::getKey) + .distinct() + .collect(Guavate.toImmutableList()); + try { + MailboxId targetId = mailboxIdFactory.fromString(patch.getMailboxIds().get().iterator().next()); + mailboxManager.moveMessages(range, mailboxId, targetId, mailboxSession); + responseBuilder.updated(messageIds); + } catch (MailboxException e) { + messageIds + .forEach(messageId -> handleMessageUpdateException(messageId, responseBuilder, e)); + } catch (IllegalArgumentException e) { + ValidationResult invalidPropertyKeywords = ValidationResult.builder() + .property(MessageProperties.MessageProperty.keywords.asFieldName()) + .message(e.getMessage()) + .build(); + + messageIds + .forEach(messageId -> handleInvalidRequest(responseBuilder, messageId, ImmutableList.of(invalidPropertyKeywords), patch)); } }); } else { diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/UpdateMessagePatch.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/UpdateMessagePatch.java index 4aa3663..519a55a 100644 --- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/UpdateMessagePatch.java +++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/model/UpdateMessagePatch.java @@ -144,6 +144,12 @@ public class UpdateMessagePatch { return !mailboxIds.isPresent() && (oldKeywords.isPresent() || keywords.isPresent()); } + public boolean isOnlyAMove() { + return mailboxIds.map(list -> list.size() == 1).orElse(false) + && oldKeywords.isEmpty() + && keywords.isEmpty(); + } + public ImmutableList<ValidationResult> getValidationErrors() { return validationErrors; } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
