[MOB] JAMES-2997 Remove byte array from attachment #3061

chibenwa · 2020-01-22T04:40:21Z

Massive effort to port our MOB into a viable pull request (wooot!)

I did my best to clean up the (very) messy history inherited from the MOC but did not managed to renabe test as part of primary work.

Arsnael

Read it

Arsnael · 2020-01-22T08:15:48Z

[ea3b88335f055649169f02ea3c5f0f3dd2958700] [ERROR] Failures: 
[ea3b88335f055649169f02ea3c5f0f3dd2958700] [ERROR]   IndexableMessageTest.hasAttachmentsShouldReturnTrueWhenPropertyIsPresentAndTrue:334 
[ea3b88335f055649169f02ea3c5f0f3dd2958700] Expecting:
[ea3b88335f055649169f02ea3c5f0f3dd2958700]  <false>
[ea3b88335f055649169f02ea3c5f0f3dd2958700] to be equal to:
[ea3b88335f055649169f02ea3c5f0f3dd2958700]  <true>
[ea3b88335f055649169f02ea3c5f0f3dd2958700] but was not.

trantienduchn · 2020-01-22T09:33:13Z

It's too big. can you split it into readable parts?

chibenwa · 2020-01-22T10:05:40Z

It's too big. can you split it into readable parts?

Note sure about how to do that: it comes as a "big ball of mud", and without disabling features, it will be hard to get that done.

rouazana

overall nice, but:

I'm pretty sure you can extract some refactoring steps into other PR
do you have some gatling tests to see if it improves thnigs?
(I've not finished reviewing all the changes)

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java

mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentTest.java

remk · 2020-01-24T09:24:48Z

...x/cassandra/src/test/java/org/apache/james/mailbox/cassandra/CassandraTestSystemFixture.java

@@ -73,7 +73,7 @@ static CassandraMailboxManager createMailboxManager(CassandraMailboxSessionMappe
        SessionProviderImpl sessionProvider = new SessionProviderImpl(mock(Authenticator.class), mock(Authorizator.class));

        QuotaComponents quotaComponents = QuotaComponents.disabled(sessionProvider, mapperFactory);
-        MessageSearchIndex index = new SimpleMessageSearchIndex(mapperFactory, mapperFactory, new DefaultTextExtractor());
+        MessageSearchIndex index = new SimpleMessageSearchIndex(mapperFactory, mapperFactory, new DefaultTextExtractor(), null);


To not bother building a complicated value.

mailbox/store/src/test/java/org/apache/james/mailbox/store/StoreBlobManagerTest.java

mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentTest.java

server/container/util/src/main/java/org/apache/james/util/io/SizeInputStream.java

.../store/src/test/java/org/apache/james/mailbox/store/mail/model/impl/PropertyBuilderTest.java

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentStorer.java

mailbox/store/src/test/java/org/apache/james/mailbox/store/mail/model/AttachmentMapperTest.java

mbaechler · 2020-01-27T10:53:35Z

server/container/util/src/main/java/org/apache/james/util/io/SizeInputStream.java

+
+import org.apache.commons.lang3.NotImplementedException;
+
+public class SizeInputStream extends InputStream {


we probably can use ByteSource for this kind of use cases

ByteSource is not applicable for any kind of InputStream (think JMAP attachment download).

Should we add a copy to a temporary file in the path?

mailbox/api/src/main/java/org/apache/james/mailbox/model/ParsedAttachment.java

...src/main/java/org/apache/james/mailbox/jpa/mail/model/openjpa/AbstractJPAMailboxMessage.java

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java

mbaechler · 2020-01-27T13:34:13Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java


        return mapperFactory.getMessageMapper(session).execute(() -> {
-            storeAttachment(message, messageAttachments, session);
+            List<MessageAttachment> attachments = storeAttachments(messageId, content, session);
+            MailboxMessage message = createMessage(internalDate, size, bodyStartOctet, content, flags, propertyBuilder, attachments);


I really think that it's not the responsibility of storeAttachments to generate the MessageAttachement ids, It makes everything much more complex than it should be.

It's a common pattern we have to have the storage layer generating ids

I don't really alternatives regarding this...

We had the same problem with blob-store recently. We decided to give the responsibility to the client. It's IMO a very important design decision.
In the case of this PR it will make some things simpler.

I guess I agree. It has not been fixed?

mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/model/impl/MessageParser.java

mbaechler · 2020-01-27T13:44:30Z

.../cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraAttachmentDAO.java

+
+        public AttachmentWithBytes(Attachment attachment, byte[] bytes) {
+            this.attachment = attachment;
+            this.bytes = bytes;


how does it relate to the commit title?

chibenwa · 2020-02-04T04:53:03Z

(forced pushed to try to get a 🍏 build... - there was a rebase conflict )

Arsnael · 2020-02-06T03:07:13Z

tests are 🍏

[5cc9699085f400f2ee5ef28c64425c8f43b492b5] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-jar-plugin:3.1.2:jar (default-jar) on project apache-james-mpt-core: Execution default-jar of goal org.apache.maven.plugins:maven-jar-plugin:3.1.2:jar failed: Unable to load the mojo 'jar' (or one of its required components) from the plugin 'org.apache.maven.plugins:maven-jar-plugin:3.1.2': com.google.inject.ProvisionException: Unable to provision, see the following errors:

test this please

chibenwa · 2020-02-06T06:49:38Z

test this please

mbaechler · 2020-02-06T14:55:54Z

mailbox/api/src/main/java/org/apache/james/mailbox/model/ParsedAttachment.java

@@ -35,7 +31,7 @@

        @FunctionalInterface
        interface RequireContent {
-            RequireName content(InputStream stream);
+            RequireName content(byte[] bytes);


the pr is named "Remove byte array from attachment " and here we put it back, apparently.
Did I missed something?

It is not an attachment stored in the mailbox-api but a result of a parsing.

Different topic! We do not retrieve these bytes on every read!

that means that parsing is no more streamed, while storing is now streamed?

Arsnael · 2020-02-07T03:13:47Z

[5cc9699085f400f2ee5ef28c64425c8f43b492b5] [ERROR] Errors: 
[5cc9699085f400f2ee5ef28c64425c8f43b492b5] [ERROR]   WebAdminUtilsTest.serverShouldBeAbletoStartConcurrently:38 » NotTerminated

test this please

chibenwa · 2020-02-07T04:38:49Z

test this please

...er/container/util/src/test/java/org/apache/james/util/io/CurrentPositionInputStreamTest.java

chibenwa · 2020-02-07T11:47:21Z

:green: wooot!

chibenwa · 2020-02-10T08:44:15Z

Conclusion: using provisionned workload, I notice a p99 GetMessages response time 60% decrease

Before

After

...er/container/util/src/test/java/org/apache/james/util/io/CurrentPositionInputStreamTest.java

rouazana · 2020-02-10T09:58:25Z

Conclusion: using provisionned workload, I notice a p99 GetMessages response time 60% decrease

Did you notice the big difference for ListMessages? It's really bad, no? (mean time 40ms -> 169ms)

chibenwa · 2020-02-11T02:28:26Z

Did you notice the big difference for ListMessages? It's really bad, no? (mean time 40ms -> 169ms)

I have no idea where it comes from to be honnest

rouazana · 2020-02-11T08:29:16Z

I have no idea where it comes from to be honnest

That's really annoying to praise a 60% improvment on the p99 while not looking at a 4x performance decrease in median and mean!

Either you try again to reproduce your issue, or it's worthless to do performance testing at this level.

chibenwa · 2020-02-11T09:33:37Z

Either you try again to reproduce your issue, or it's worthless to do performance testing at this level.

Agreed I need to redo it, that's why I did not remove the label, btw.

…ning search

… messageAttachments

…ageAttachmentMetadata

… message has attachment

…ne if a message has attachment

chibenwa · 2020-04-21T04:21:33Z

Forced pushed to solve conflict and remove charset commit.

chibenwa · 2020-04-21T04:38:20Z

#3316 discusses the charset issue fixing (if needed) separatly.

chibenwa · 2020-04-21T04:42:05Z

I still have a question regarding performance (which is the main point of this PR).
Looking at #3061 (comment) and #3061 (comment) the after results are very similar, but the before results are very different. Why?

Running perf tests on hosted VMs can trivially explain the difference between two runs separated in time (several hours difference between the two runs in the first one).

Do we have a plan for a perf testing platform?

Also, the results of run #2 are coherent with the expectations we can have following the changes performed (p99 improvment of GetMessages only, but important). It can be trivially explained by the fact we no longer load useless attachments.

Thus I have no concerns accepting the second one.

If you still have concerns, is me running systematically perf tests useful, or am I just wasting my time?

rouazana · 2020-04-21T07:23:54Z

If you still have concerns, is me running systematically perf tests useful, or am I just wasting my time?

Do you think my goal is to make you wasting time?

I see some (very) strange things, I ask for explanation.

If I look at the issue globally, either this PR is causing some performance troubles of getMessageList in this context, or we have globally a performance issue on current master. I would like that we dig into the issue instead of grumbling about VMs.

Arsnael · 2020-04-21T07:27:39Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageStorer.java

     */
    Pair<MessageMetaData, Optional<List<MessageAttachmentMetadata>>> appendMessageToStore(Mailbox mailbox, Date internalDate, int size, int bodyStartOctet, SharedInputStream content, Flags flags, PropertyBuilder propertyBuilder, MailboxSession session) throws MailboxException;

+    /**
+     * MessageStorer to parsing, storing and returning AttachmentMetadata


or remove theto, or don't conjugate the verbs parse, store and return

mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageStorer.java

rouazana

some details now, it's globally ok for me even if I'm worry about the performace issue

rouazana · 2020-04-21T07:26:25Z

mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageStorer.java

+     * If supported by the underlying implementation, this method will parse the messageContent to retrieve associated
+     * attachments and will store them.
+     *
+     * Otherwize an empty optional will be returned on the right side of the pair.
     */
    Pair<MessageMetaData, Optional<List<MessageAttachmentMetadata>>> appendMessageToStore(Mailbox mailbox, Date internalDate, int size, int bodyStartOctet, SharedInputStream content, Flags flags, PropertyBuilder propertyBuilder, MailboxSession session) throws MailboxException;


if it's at the implementation level, why not Optional<Pair<MessageMetaData, List<MessageAttachmentMetadata>> ?

You always have the message metadata but the underlying implementation might not return you attachment metadata (because it does not support it)

mailbox/store/src/main/java/org/apache/james/mailbox/store/MessageStorer.java

chibenwa · 2020-04-21T07:35:27Z

If I look at the issue globally, either this PR is causing some performance troubles of getMessageList in this context, or we have globally a performance issue on current master. I would like that we dig into the issue instead of grumbling about VMs.

#3266 (comment) adds significant performance gains by moving GetMessageList into reactive style

Before

After

Looking at the perf test result not benefiting (amongst others) about this improvment, differences in performance can be explained by the different load (not doing 5 GetMessages)

This seems consistent to me.

And we don't have performance issue on GetMessageList on master as #3266 (comment) proves it.

chibenwa · 2020-04-21T07:56:55Z

some details now, it's globally ok for me even if I'm worry about the performace issue

Again, it's not exactly the same load than the other (recent) perf tests you have been seeing and the latest test on this lack most of the recent optimization: I'm not worry.

chibenwa · 2020-04-22T02:38:40Z

Merged

chibenwa added perf Contributes some performance enhencements requires-load-testing Running load testing to back these changes is required for adoption labels Jan 22, 2020

chibenwa added this to the Sprint 14 - Robusta beans milestone Jan 22, 2020

chibenwa self-assigned this Jan 22, 2020

chibenwa mentioned this pull request Jan 22, 2020

JAMES-2997 Split byte array away from Attachment #2943

Closed

chibenwa force-pushed the mob-programming-remove-byte-array-from-attachment branch 2 times, most recently from 9e30bde to ea3b883 Compare January 22, 2020 06:39

Arsnael reviewed Jan 22, 2020

View reviewed changes

rouazana reviewed Jan 22, 2020

View reviewed changes

mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java Outdated Show resolved Hide resolved

mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentTest.java Show resolved Hide resolved

remk reviewed Jan 24, 2020

View reviewed changes

mbaechler reviewed Jan 27, 2020

View reviewed changes

chibenwa force-pushed the mob-programming-remove-byte-array-from-attachment branch from 2067ed0 to 8e383a3 Compare February 4, 2020 04:52

mbaechler reviewed Feb 6, 2020

View reviewed changes

remk reviewed Feb 7, 2020

View reviewed changes

...er/container/util/src/test/java/org/apache/james/util/io/CurrentPositionInputStreamTest.java Outdated Show resolved Hide resolved

Arsnael reviewed Feb 10, 2020

View reviewed changes

chibenwa force-pushed the mob-programming-remove-byte-array-from-attachment branch from 28e9d85 to 974ac25 Compare February 28, 2020 03:43

chibenwa added 15 commits April 21, 2020 11:20

JAMES-2997 ReactorUtils::toChunk should be done on an elastic scheduler

6fa18cc

fixup! JAMES-2990 s/SizeInputStream/CurrentPositionInputStream/

cb5807c

fixup! JAMES-2997 step #1 Rely on AttachmentContentLoader within scan…

d3d2ac7

…ning search

fixup! JAMES-2997 step #1 Rely on AttachmentContentLoader within scan…

1b950f3

…ning search

fixup! JAMES-2997 step #1 Rely on AttachmentContentLoader within scan…

745116f

…ning search

fixup! JAMES-2997 step #1 Rely on AttachmentContentLoader within scan…

c1dc46a

…ning search

fixup! JAMES-2997 step #2 Remove Attachment::toBlob method

52d7aaf

fixup! JAMES-2997 step #10 Mailbox backend is responsible of creating…

08a73b1

… messageAttachments

JAMES-2997 s/Attachment/AttachmentMetadata + s/MessageAttachment/Mess…

5960b91

…ageAttachmentMetadata

fixup! JAMES-2997 step #11 Remove bytes from attachment

959c969

fixup! JAMES-2997 Renable MIMEMessageConverterTest

78b6381

fixup! JAMES-2997 Renable SetMessagesMethodTest

8f4958b

fixup! JAMES-2997 step #14 Abstract message storage

4042565

fixup! JAMES-2997 step #7 Do not rely on properties to determine if a…

5e668d2

… message has attachment

fixup! fixup! JAMES-2997 step #7 Do not rely on properties to determi…

147fb9b

…ne if a message has attachment

chibenwa force-pushed the mob-programming-remove-byte-array-from-attachment branch from d111a90 to 147fb9b Compare April 21, 2020 04:21

chibenwa added a commit to chibenwa/james-project that referenced this pull request Apr 21, 2020

[NO_REVIEW] linagora#3061 JAMES-2997 Remove byte array from attachment

55c78fc

chibenwa mentioned this pull request Apr 21, 2020

JAMES-2997 Extraction of charset cary over in Attachment content type #3316

Closed

Arsnael approved these changes Apr 21, 2020

View reviewed changes

rouazana approved these changes Apr 21, 2020

View reviewed changes

fixup! fixup! JAMES-2997 step #14 Abstract message storage

e55ad1b

chibenwa added the waiting_merge We are about to merge this! label Apr 21, 2020

chibenwa closed this Apr 22, 2020


		import org.apache.commons.lang3.NotImplementedException;

		public class SizeInputStream extends InputStream {

[MOB] JAMES-2997 Remove byte array from attachment #3061

[MOB] JAMES-2997 Remove byte array from attachment #3061

Conversation

chibenwa commented Jan 22, 2020

Arsnael left a comment

Choose a reason for hiding this comment

Arsnael commented Jan 22, 2020

trantienduchn commented Jan 22, 2020

chibenwa commented Jan 22, 2020

rouazana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Feb 4, 2020 • edited Loading

Arsnael commented Feb 6, 2020

chibenwa commented Feb 6, 2020

Choose a reason for hiding this comment

chibenwa Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arsnael commented Feb 7, 2020

chibenwa commented Feb 7, 2020

chibenwa commented Feb 7, 2020

chibenwa commented Feb 10, 2020

rouazana commented Feb 10, 2020

chibenwa commented Feb 11, 2020

rouazana commented Feb 11, 2020

chibenwa commented Feb 11, 2020

chibenwa commented Apr 21, 2020

chibenwa commented Apr 21, 2020

chibenwa commented Apr 21, 2020

rouazana commented Apr 21, 2020

Choose a reason for hiding this comment

rouazana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Apr 21, 2020 • edited Loading

chibenwa commented Apr 21, 2020

chibenwa commented Apr 22, 2020

chibenwa commented Feb 4, 2020 •

edited

Loading

chibenwa Feb 6, 2020 •

edited

Loading

chibenwa commented Apr 21, 2020 •

edited

Loading