You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/02/25 08:36:30 UTC
[GitHub] [james-project] chibenwa opened a new pull request #309: Do not copy messages in memory before saving them to the blob store
chibenwa opened a new pull request #309:
URL: https://github.com/apache/james-project/pull/309
- This reduce memory impact for enqueueing emails - SMTP, JMAP message submission
- This reduce memory impact of dequeuing emails - mailetcontainer processing
- This reduce overhead of CassandraMailRepository (minor gains)
The memory gain will be major upon big messages.
Unefficient memory usage of MailboxMessage is not addressed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787812416
> @chibenwa it's quite big, I couldn't complete the review yet but I wanted to let you know I'm highly interested.
Quite big but I did try to keep a clear history...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa edited a comment on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-786358253
1st commit is :green_apple: on the CI
I run performance tests for this PR...
## Before
## After
## Conclusion
Little performance enhancement. Great p99 diminition. More importantly we can notice that no "stop the world GC was triggered" during long period of time, as in the first run.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584594406
##########
File path: server/blob/blob-storage-strategy/src/main/scala/org/apache/james/server/blob/deduplication/PassThroughBlobStore.scala
##########
@@ -51,6 +52,15 @@ class PassThroughBlobStore @Inject()(blobStoreDAO: BlobStoreDAO,
.`then`(SMono.just(blobId))
}
+ override def save(bucketName: BucketName, data: ByteSource, storagePolicy: BlobStore.StoragePolicy): Publisher[BlobId] = {
Review comment:
usually, starting at the third copy/paste, I factorize the code.
##########
File path: mailbox/api/src/main/java/org/apache/james/mailbox/MessageManager.java
##########
@@ -242,6 +242,11 @@ public Builder withInternalDate(Date date) {
return this;
}
+ public Builder withInternalDate(Optional<Date> date) {
Review comment:
can we put this commit in another PR please?
##########
File path: mailbox/store/src/test/java/org/apache/james/mailbox/store/mail/model/AttachmentMapperTest.java
##########
@@ -159,7 +160,7 @@ void getRelatedMessageIdsShouldReturnMessageIdWhenStoredWithMessageId() throws E
MessageId messageId = generateMessageId();
AttachmentId attachmentId = attachmentMapper.storeAttachmentsForMessage(ImmutableList.of(ParsedAttachment.builder()
.contentType("content")
- .content("".getBytes(StandardCharsets.UTF_8))
+ .content(ByteSource.wrap("".getBytes(StandardCharsets.UTF_8)))
Review comment:
it's a very complex way of providing an empty ByteSource
##########
File path: server/blob/blob-cassandra/src/test/java/org/apache/james/blob/cassandra/cache/CachedBlobStoreTest.java
##########
@@ -116,90 +116,90 @@ public void shouldCacheWhenDefaultBucketName() {
public void shouldNotCacheWhenNotDefaultBucketName() {
BlobId blobId = Mono.from(testee().save(TEST_BUCKETNAME, EIGHT_KILOBYTES, SIZE_BASED)).block();
- SoftAssertions.assertSoftly(soflty -> {
- soflty.assertThat(Mono.from(cache.read(blobId)).blockOptional()).isEmpty();
- soflty.assertThat(Mono.from(backend.readBytes(TEST_BUCKETNAME, blobId)).block()).containsExactly(EIGHT_KILOBYTES);
+ SoftAssertions.assertSoftly(softly -> {
Review comment:
could you move this commit to another PR please?
##########
File path: mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
##########
@@ -321,27 +321,17 @@ public AppendResult appendMessage(InputStream msgIn, Date internalDate, final Ma
// source for the InputStream
file = File.createTempFile("imap", ".msg");
try (FileOutputStream out = new FileOutputStream(file);
- BufferedOutputStream bufferedOut = new BufferedOutputStream(out);
Review comment:
can we put this commit in another PR?
##########
File path: server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
##########
@@ -135,6 +134,18 @@ public Response onLine(SMTPSession session, ByteBuffer lineByteBuffer, LineHandl
return null;
}
+ private byte[] readBytes(ByteBuffer line) {
Review comment:
there's no existing method to read a bytebuffer to an array?
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CachedBlobStore.java
##########
@@ -210,6 +212,23 @@ public InputStream read(BucketName bucketName, BlobId blobId) {
return backend.save(bucketName, inputStream, storagePolicy);
}
+ @Override
+ public Publisher<BlobId> save(BucketName bucketName, ByteSource byteSource, StoragePolicy storagePolicy) {
+ Preconditions.checkNotNull(byteSource, "ByteSource must not be null");
+
+ if (isAbleToCache(bucketName, storagePolicy)) {
+ return Mono.from(backend.save(bucketName, byteSource, storagePolicy))
Review comment:
why don't we call that `saveInCache` as for other `save` methods?
##########
File path: server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/delivery/MailboxAppender.java
##########
@@ -78,8 +77,7 @@ private AppendResult append(MimeMessage mail, Username user, String folder, Mail
}
private AppendResult appendMessageToMailbox(MimeMessage mail, MailboxSession session, MailboxPath path) throws MailboxException, MessagingException {
- createMailboxIfNotExist(session, path);
- final MessageManager mailbox = mailboxManager.getMailbox(path, session);
+ MessageManager mailbox = createMailboxIfNotExist(session, path);
Review comment:
can we put that commit into another PR please?
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageWrapperTest.java
##########
@@ -292,4 +294,14 @@ public void saveChangesShouldPreserveMessageId() throws Exception {
assertThat(mimeMessageWrapper.getMessageID())
.isEqualTo(messageId);
}
+
+ @Test
+ public void test() throws Exception {
Review comment:
could we have a proper test name?
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
there's no guarantee that:
* the buffer won't be recycled by another layer
* the array has the size of the data
##########
File path: mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
##########
@@ -347,6 +360,24 @@ public AppendResult appendMessage(InputStream msgIn, Date internalDate, final Ma
}
}
+ public AppendResult appendMessage(Content msgIn, Date internalDate, final MailboxSession mailboxSession, boolean isRecent, Flags flagsToBeSet) throws MailboxException {
Review comment:
that's quite a bunch of logic, is it tested in some way?
##########
File path: server/protocols/protocols-lmtp/src/main/java/org/apache/james/lmtpserver/hook/MailboxDeliverToRecipientHandler.java
##########
@@ -80,15 +82,25 @@ public HookResult deliver(SMTPSession session, MailAddress recipient, MailEnvelo
mailboxManager.getMailbox(MailboxPath.inbox(username), mailboxSession)
.appendMessage(MessageManager.AppendCommand.builder()
.recent()
- .build(envelope.getMessageInputStream()),
+ .build(new Content() {
Review comment:
I didn't go back to check all `Content` instances but it looks to be a lot of inline classes definitions that are exactly the same
##########
File path: mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
##########
@@ -439,9 +470,10 @@ private int getBodyStartOctet(BodyOffsetInputStream bIn) {
return bodyStartOctet;
}
- private AppendResult createAndDispatchMessage(Date internalDate, MailboxSession mailboxSession, File file, PropertyBuilder propertyBuilder, Flags flags, int bodyStartOctet) throws IOException, MailboxException {
- try (SharedFileInputStream contentIn = new SharedFileInputStream(file)) {
- final int size = (int) file.length();
+ private AppendResult createAndDispatchMessage(Date internalDate, MailboxSession mailboxSession, Content content, PropertyBuilder propertyBuilder, Flags flags, int bodyStartOctet) throws IOException, MailboxException {
+ // TODO MailboxMessage should be backed by a "content"
+ try (SharedFileInputStream contentIn = null) {
Review comment:
this commit breaks that code on purpose? is it fixed later?
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageWrapper.java
##########
@@ -417,7 +417,7 @@ public long getMessageSize() throws MessagingException {
}
} else if (source != null && !bodyModified) {
try (InternetHeadersInputStream headerStream = new InternetHeadersInputStream(headers)) {
- return source.getMessageSize() - initialHeaderSize + IOUtils.consume(headerStream);
+ return source.getMessageSize() - initialHeaderSize + IOUtils.consume(headerStream) -2;
Review comment:
please avoid such magic number
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-789478567
:green_apple: build
I will rebase, clean up the history and undraft this
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584631246
##########
File path: server/blob/blob-storage-strategy/src/main/scala/org/apache/james/server/blob/deduplication/PassThroughBlobStore.scala
##########
@@ -51,6 +52,15 @@ class PassThroughBlobStore @Inject()(blobStoreDAO: BlobStoreDAO,
.`then`(SMono.just(blobId))
}
+ override def save(bucketName: BucketName, data: ByteSource, storagePolicy: BlobStore.StoragePolicy): Publisher[BlobId] = {
Review comment:
I'd say let's drop the InputSream method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r587981670
##########
File path: server/data/data-jdbc/src/test/java/org/apache/james/mailrepository/jdbc/JDBCMailRepositoryTest.java
##########
@@ -85,4 +88,12 @@ public void storeRegularMailShouldNotFailWhenNullSender() {
public void shouldPreserveDsnParameters() {
}
+
+ @Override
+ @Test
+ @Tag(Unstable.TAG)
+ // Isolation issues
+ public void sizeShouldReturnZeroWhenEmpty() {
Review comment:
If I'm not wrong, omit the tag, if you override a test with an empty body, you just don't play any test at all 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-792482733
Merged.
If there is some refactorings to do on this work I can handle them in another PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa closed pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa closed pull request #309:
URL: https://github.com/apache/james-project/pull/309
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585561854
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
Let's say I'm wrong (because you look really sure of your opinion). Then open ByteBuffer.wrap and tell me what happen when offset/limit is used. Do you see any shrinking of the buffer somewhere?
I can remember being bitten by this in the past, please look closer to this problem because I'm confident it's not correct code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r588208524
##########
File path: server/data/data-jdbc/src/test/java/org/apache/james/mailrepository/jdbc/JDBCMailRepositoryTest.java
##########
@@ -79,21 +79,21 @@ private BasicDataSource getDataSource() {
@Override
@Disabled("JAMES-2546 This mail repository does not support null sender")
- public void storeRegularMailShouldNotFailWhenNullSender() {
-
+ public void storeRegularMailShouldNotFailWhenNullSender() throws Exception {
+ MailRepositoryContract.super.storeRegularMailShouldNotFailWhenNullSender();
Review comment:
It does not hurt to have a body when disabled as far as I know
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-786358253
1st commit is :green_apple: on the CI
I run performance tests for it...
## Before
## After
## Conclusion
Little performance enhancement. Great p99 diminition. More importantly we can notice that no "stop the world GC was triggered" during long period of time, as in the first run.
I will continue looking for places where to avoid memory copies, will fix compilation of the last commit, and continue benches ;-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585592334
##########
File path: mailbox/store/src/test/java/org/apache/james/mailbox/store/mail/model/AttachmentMapperTest.java
##########
@@ -159,7 +160,7 @@ void getRelatedMessageIdsShouldReturnMessageIdWhenStoredWithMessageId() throws E
MessageId messageId = generateMessageId();
AttachmentId attachmentId = attachmentMapper.storeAttachmentsForMessage(ImmutableList.of(ParsedAttachment.builder()
.contentType("content")
- .content("".getBytes(StandardCharsets.UTF_8))
+ .content(ByteSource.wrap("".getBytes(StandardCharsets.UTF_8)))
Review comment:
Fair
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r588202147
##########
File path: server/data/data-jdbc/src/test/java/org/apache/james/mailrepository/jdbc/JDBCMailRepositoryTest.java
##########
@@ -79,21 +79,21 @@ private BasicDataSource getDataSource() {
@Override
@Disabled("JAMES-2546 This mail repository does not support null sender")
- public void storeRegularMailShouldNotFailWhenNullSender() {
-
+ public void storeRegularMailShouldNotFailWhenNullSender() throws Exception {
+ MailRepositoryContract.super.storeRegularMailShouldNotFailWhenNullSender();
Review comment:
If it's disabled it does not matter to have a body, as it will not be played. My previous comment was for the one with Unstable Tag, as those tests are still played, but in a separate phase of the build
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584631246
##########
File path: server/blob/blob-storage-strategy/src/main/scala/org/apache/james/server/blob/deduplication/PassThroughBlobStore.scala
##########
@@ -51,6 +52,15 @@ class PassThroughBlobStore @Inject()(blobStoreDAO: BlobStoreDAO,
.`then`(SMono.just(blobId))
}
+ override def save(bucketName: BucketName, data: ByteSource, storagePolicy: BlobStore.StoragePolicy): Publisher[BlobId] = {
Review comment:
I'd say let's drop the InputSream method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584637491
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
Dropped
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
see https://github.com/apache/james-project/pull/316
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r589171776
##########
File path: server/data/data-jdbc/src/test/java/org/apache/james/mailrepository/jdbc/JDBCMailRepositoryTest.java
##########
@@ -79,21 +79,21 @@ private BasicDataSource getDataSource() {
@Override
@Disabled("JAMES-2546 This mail repository does not support null sender")
- public void storeRegularMailShouldNotFailWhenNullSender() {
-
+ public void storeRegularMailShouldNotFailWhenNullSender() throws Exception {
+ MailRepositoryContract.super.storeRegularMailShouldNotFailWhenNullSender();
Review comment:
It does not, just useless. But I don't mind TBH
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] jeantil commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787803084
@chibenwa it's quite big, I couldn't complete the review yet but I wanted to let you know I'm highly interested.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585550803
##########
File path: server/blob/blob-storage-strategy/src/main/scala/org/apache/james/server/blob/deduplication/PassThroughBlobStore.scala
##########
@@ -51,6 +52,15 @@ class PassThroughBlobStore @Inject()(blobStoreDAO: BlobStoreDAO,
.`then`(SMono.just(blobId))
}
+ override def save(bucketName: BucketName, data: ByteSource, storagePolicy: BlobStore.StoragePolicy): Publisher[BlobId] = {
Review comment:
> Fixups welcomed ;-)
Come on, if you don't want the code reviewed, just say it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa edited a comment on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-786358253
1st commit is :green_apple: on the CI
I run performance tests for this PR...
## Before
## After
## Conclusion
Little performance enhancement. Great p99 diminition. More importantly we can notice that no "stop the world GC was triggered" during long period of time, as in the first run.
I will continue looking for places where to avoid memory copies, will fix compilation of the last commit, and continue benches ;-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-790328553
:green_apple: build.
I would like to merge this reasonably soon as is ships nice enhancements.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584631489
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CachedBlobStore.java
##########
@@ -210,6 +212,23 @@ public InputStream read(BucketName bucketName, BlobId blobId) {
return backend.save(bucketName, inputStream, storagePolicy);
}
+ @Override
+ public Publisher<BlobId> save(BucketName bucketName, ByteSource byteSource, StoragePolicy storagePolicy) {
+ Preconditions.checkNotNull(byteSource, "ByteSource must not be null");
+
+ if (isAbleToCache(bucketName, storagePolicy)) {
+ return Mono.from(backend.save(bucketName, byteSource, storagePolicy))
Review comment:
Because the params are not exactly the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584630317
##########
File path: server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
##########
@@ -135,6 +134,18 @@ public Response onLine(SMTPSession session, ByteBuffer lineByteBuffer, LineHandl
return null;
}
+ private byte[] readBytes(ByteBuffer line) {
Review comment:
Nope not that I know>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787203125
Broken tests:
```
org.apache.james.transport.mailets.AddFooterTest.shouldAddFooterWhenMultipartMessage
org.apache.james.transport.mailets.ICSAttachmentWorkflowTest.mailShouldNotContainCalendarContentInTextBodyButAttachment
& JPA MPT tests
```
I will investigate them...
Perf wise:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584628642
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageWrapperTest.java
##########
@@ -292,4 +294,14 @@ public void saveChangesShouldPreserveMessageId() throws Exception {
assertThat(mimeMessageWrapper.getMessageID())
.isEqualTo(messageId);
}
+
+ @Test
+ public void test() throws Exception {
Review comment:
Yes for sure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584629804
##########
File path: mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
##########
@@ -347,6 +360,24 @@ public AppendResult appendMessage(InputStream msgIn, Date internalDate, final Ma
}
}
+ public AppendResult appendMessage(Content msgIn, Date internalDate, final MailboxSession mailboxSession, boolean isRecent, Flags flagsToBeSet) throws MailboxException {
Review comment:
Yes in Manager tests, refactorings were automated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787867697
>However, I would like all non-directly related changes to be moved into a PR to make this one more accessible to people wanting to read it.
Most of the changes are related.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584635867
##########
File path: mailbox/api/src/main/java/org/apache/james/mailbox/MessageManager.java
##########
@@ -242,6 +242,11 @@ public Builder withInternalDate(Date date) {
return this;
}
+ public Builder withInternalDate(Optional<Date> date) {
Review comment:
No it is directly related.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584630085
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
We likely can drop this commit if you wish.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584629445
##########
File path: server/protocols/protocols-lmtp/src/main/java/org/apache/james/lmtpserver/hook/MailboxDeliverToRecipientHandler.java
##########
@@ -80,15 +82,25 @@ public HookResult deliver(SMTPSession session, MailAddress recipient, MailEnvelo
mailboxManager.getMailbox(MailboxPath.inbox(username), mailboxSession)
.appendMessage(MessageManager.AppendCommand.builder()
.recent()
- .build(envelope.getMessageInputStream()),
+ .build(new Content() {
Review comment:
a lot of inline classes definitions that looks exactly the same. Nuance. I generalized where possible.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585549994
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CachedBlobStore.java
##########
@@ -210,6 +212,23 @@ public InputStream read(BucketName bucketName, BlobId blobId) {
return backend.save(bucketName, inputStream, storagePolicy);
}
+ @Override
+ public Publisher<BlobId> save(BucketName bucketName, ByteSource byteSource, StoragePolicy storagePolicy) {
+ Preconditions.checkNotNull(byteSource, "ByteSource must not be null");
+
+ if (isAbleToCache(bucketName, storagePolicy)) {
+ return Mono.from(backend.save(bucketName, byteSource, storagePolicy))
Review comment:
I mean, you could create a method named `saveInCache` to make the code clearer _as we did for other methods_
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585592117
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
To be fair, from what I see we could use the offset and the length to generate a stream on demand without an array copy.
Here the callers need a byte[] access.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584591290
##########
File path: server/blob/blob-api/src/main/java/org/apache/james/blob/api/BlobStore.java
##########
@@ -35,6 +37,8 @@
Publisher<BlobId> save(BucketName bucketName, InputStream data, StoragePolicy storagePolicy);
+ Publisher<BlobId> save(BucketName bucketName, ByteSource data, StoragePolicy storagePolicy);
Review comment:
Maybe we need an API equivalent to `ByteSource` of our own here (called eg `Blob`) providing content and length.
Feedback?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584701283
##########
File path: server/blob/blob-storage-strategy/src/main/scala/org/apache/james/server/blob/deduplication/PassThroughBlobStore.scala
##########
@@ -51,6 +52,15 @@ class PassThroughBlobStore @Inject()(blobStoreDAO: BlobStoreDAO,
.`then`(SMono.just(blobId))
}
+ override def save(bucketName: BucketName, data: ByteSource, storagePolicy: BlobStore.StoragePolicy): Publisher[BlobId] = {
Review comment:
Fixups welcomed ;-)
The code is simple, easy to understand, tested, vary subtly in inputs and cannot easily find a way to factorize it hence will not spend my time on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-786753049
As off 6743c2a
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584630317
##########
File path: server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
##########
@@ -135,6 +134,18 @@ public Response onLine(SMTPSession session, ByteBuffer lineByteBuffer, LineHandl
return null;
}
+ private byte[] readBytes(ByteBuffer line) {
Review comment:
Nope not that I know
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
Dropped
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-786019955
The message is then represented as a byte array only when reading it from the mailbox...
I am running some SMTP benchmarks on this...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585597438
##########
File path: server/blob/blob-storage-strategy/src/main/scala/org/apache/james/server/blob/deduplication/PassThroughBlobStore.scala
##########
@@ -51,6 +52,15 @@ class PassThroughBlobStore @Inject()(blobStoreDAO: BlobStoreDAO,
.`then`(SMono.just(blobId))
}
+ override def save(bucketName: BucketName, data: ByteSource, storagePolicy: BlobStore.StoragePolicy): Publisher[BlobId] = {
Review comment:
Come on, I ask kindly for suggestions, and you give none...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584671602
##########
File path: server/blob/blob-cassandra/src/main/java/org/apache/james/blob/cassandra/cache/CassandraBlobStoreCache.java
##########
@@ -121,6 +121,9 @@ private ByteBuffer toByteBuffer(byte[] bytes) {
private byte[] toByteArray(Row row) {
ByteBuffer byteBuffer = row.getBytes(DATA);
+ if (byteBuffer.hasArray()) {
+ return byteBuffer.array();
Review comment:
BTW you are wrong:
https://docs.oracle.com/javase/7/docs/api/java/nio/ByteBuffer.html#hasArray()
```
Tells whether or not this buffer is backed by an accessible byte array.
If this method returns true then the array and arrayOffset methods may safely be invoked.
```
https://docs.oracle.com/javase/7/docs/api/java/nio/ByteBuffer.html#array()
```
Returns the byte array that backs this buffer (optional operation).
```
As I understand that you feel concerned I will open a separated PR, But still want this code merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585551261
##########
File path: mailbox/store/src/test/java/org/apache/james/mailbox/store/mail/model/AttachmentMapperTest.java
##########
@@ -159,7 +160,7 @@ void getRelatedMessageIdsShouldReturnMessageIdWhenStoredWithMessageId() throws E
MessageId messageId = generateMessageId();
AttachmentId attachmentId = attachmentMapper.storeAttachmentsForMessage(ImmutableList.of(ParsedAttachment.builder()
.contentType("content")
- .content("".getBytes(StandardCharsets.UTF_8))
+ .content(ByteSource.wrap("".getBytes(StandardCharsets.UTF_8)))
Review comment:
`ByteSource.empty()` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa edited a comment on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787867697
>However, I would like all non-directly related changes to be moved into a PR to make this one more accessible to people wanting to read it.
Most of the changes are related. Or already have PRs.
I'll take a closer look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787440099
```
Test Result (5 failures / -74)
org.apache.james.mailrepository.jdbc.JDBCMailRepositoryTest.storingMessageWithPerRecipientHeadersShouldAllowMultipleHeadersPerUser
org.apache.james.mailrepository.jdbc.JDBCMailRepositoryTest.retrieveShouldGetStoredMail
org.apache.james.mailrepository.jdbc.JDBCMailRepositoryTest.storingMessageWithSameKeyTwiceShouldUpdateMessageContent
org.apache.james.mailrepository.jdbc.JDBCMailRepositoryTest.storingMessageWithSameKeyTwiceShouldUpdateMessageAttributes
org.apache.james.mailrepository.jdbc.JDBCMailRepositoryTest.retrieveShouldReturnAllMailProperties
```
The freaks are out ;-)
I also have some JMAP benchs:
Before:
After:
Sending emails via JMAP is thus significantly faster (better p99, meantime and throughtput)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa edited a comment on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787867697
>However, I would like all non-directly related changes to be moved into a PR to make this one more accessible to people wanting to read it.
Most of the changes are related. Or already have PRs.
I'll take a closer look.
BTW it is a Work In Progress draft ;-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787823204
Adding https://github.com/apache/james-project/pull/211 to the mix:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584424970
##########
File path: server/blob/blob-cassandra/src/test/java/org/apache/james/blob/cassandra/cache/CachedBlobStoreTest.java
##########
@@ -171,6 +172,16 @@ public void shouldCacheWhenEmptyStream() {
});
}
+ @Test
+ public void shouldCacheWhenEmptyByteSource() {
+ BlobId blobId = Mono.from(testee().save(DEFAULT_BUCKETNAME, ByteSource.wrap(EMPTY_BYTEARRAY), SIZE_BASED)).block();
+
+ SoftAssertions.assertSoftly(soflty -> {
Review comment:
s/soflty/softly
##########
File path: server/blob/blob-cassandra/src/test/java/org/apache/james/blob/cassandra/cache/CachedBlobStoreTest.java
##########
@@ -193,6 +204,18 @@ public void shouldCacheWhenFiveKilobytesSteam() {
});
}
+ @Test
+ public void shouldCacheWhenFiveKilobytesByteSource() {
+ BlobId blobId = Mono.from(testee().save(DEFAULT_BUCKETNAME, ByteSource.wrap(APPROXIMATELY_FIVE_KILOBYTES), SIZE_BASED)).block();
+
+ SoftAssertions.assertSoftly(soflty -> {
Review comment:
idem
##########
File path: server/blob/blob-cassandra/src/test/java/org/apache/james/blob/cassandra/cache/CachedBlobStoreTest.java
##########
@@ -292,6 +315,30 @@ public void shouldNotCacheWhenReadWithBigStream() {
});
}
+ @Test
+ public void shouldNotCacheWhenReadWithOutDefaultBucketByteSource() {
+ BlobId blobId = Mono.from(backend.save(TEST_BUCKETNAME, ByteSource.wrap(APPROXIMATELY_FIVE_KILOBYTES), SIZE_BASED)).block();
+
+ SoftAssertions.assertSoftly(soflty -> {
+ soflty.assertThat(Mono.from(cache.read(blobId)).blockOptional()).isEmpty();
+ soflty.assertThat(testee().read(TEST_BUCKETNAME, blobId, HIGH_PERFORMANCE))
+ .hasSameContentAs(new ByteArrayInputStream(APPROXIMATELY_FIVE_KILOBYTES));
+ soflty.assertThat(Mono.from(cache.read(blobId)).blockOptional()).isEmpty();
+ });
+ }
+
+ @Test
+ public void shouldNotCacheWhenReadWithBigByteSource() {
+ BlobId blobId = Mono.from(backend.save(DEFAULT_BUCKETNAME, ByteSource.wrap(TWELVE_MEGABYTES), SIZE_BASED)).block();
+
+ SoftAssertions.assertSoftly(soflty -> {
Review comment:
idem
##########
File path: server/blob/blob-cassandra/src/test/java/org/apache/james/blob/cassandra/cache/CachedBlobStoreTest.java
##########
@@ -292,6 +315,30 @@ public void shouldNotCacheWhenReadWithBigStream() {
});
}
+ @Test
+ public void shouldNotCacheWhenReadWithOutDefaultBucketByteSource() {
+ BlobId blobId = Mono.from(backend.save(TEST_BUCKETNAME, ByteSource.wrap(APPROXIMATELY_FIVE_KILOBYTES), SIZE_BASED)).block();
+
+ SoftAssertions.assertSoftly(soflty -> {
Review comment:
idem
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageWrapper.java
##########
@@ -361,13 +361,8 @@ public int getSize() throws MessagingException {
throw new MessagingException("Unable to calculate message size");
}
} else {
- if (!messageParsed) {
- loadMessage();
- }
-
- return super.getSize();
+ return -1;
Review comment:
-1 standing for what here?
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageWrapperTest.java
##########
@@ -256,15 +256,15 @@ public void testSizeModifiedHeaders() throws MessagingException {
public void testSizeModifiedBodyWithoutSave() throws MessagingException {
String newBody = "This is the new body of the message";
mw.setText(newBody);
- assertThat(mw.getSize()).isEqualTo(body.length());
+ assertThat(mw.getSize()).isEqualTo(-1);
}
@Test
public void testSizeModifiedBodyWithSave() throws MessagingException {
String newBody = "This is the new body of the message";
mw.setText(newBody);
mw.saveChanges();
- assertThat(mw.getSize()).isEqualTo(body.length());
+ assertThat(mw.getSize()).isEqualTo(-1);
Review comment:
Are those test names still making sense with that -1 returned?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-791099286
> I would like to merge this reasonably soon as is ships nice enhancements.
Then undraft it please. As long as it's in draft state it's not clear if the work is finished or not
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #309:
URL: https://github.com/apache/james-project/pull/309#issuecomment-787406213
Here we are! Finnally some real improvments!
Almost x3 throughtput on SMTP tests!
Interestingly, running `$ dstat -lrvn 10` on the testing server:
```
---load-avg--- --io/total- ---procs--- ------memory-usage----- ---paging-- -dsk/total- ---system-- ----total-cpu-usage---- -net/total-
1m 5m 15m | read writ|run blk new| used buff cach free| in out | read writ| int csw |usr sys idl wai hiq siq| recv send
10.5 10.6 10.5|47.2 339 |6.5 0.5 2.6|15.4G 4240k 13.1G 262M| 0 0 | 760k 48M| 21k 35k| 31 12 53 3 0 2| 25M 38M
9.28 10.3 10.4|39.1 474 |3.2 0.5 2.9|15.5G 4144k 13.1G 228M| 0 0 | 593k 49M| 16k 26k| 24 10 62 3 0 1| 22M 32M
10.3 10.5 10.5|48.3 617 |6.1 0.1 12|15.5G 3884k 13.1G 249M| 0 0 | 743k 62M| 22k 38k| 31 14 52 2 0 2| 28M 41M
9.30 10.3 10.4|48.1 318 |4.8 0.5 4.7|15.5G 3808k 13.1G 231M| 0 0 | 914k 48M| 18k 30k| 28 11 57 2 0 1| 23M 34M
8.86 10.1 10.3|43.2 315 |5.6 0.2 2.9|15.4G 3716k 13.0G 323M| 0 0 | 732k 47M| 19k 33k| 30 12 55 2 0 2| 25M 37M
```
We can notice that we are massively writing to the disk (~50MB/s) but almost not reading. Previous runs were hitting 50MB/s reads + ~100MB/s writes. I assume we now better leverage the FileSystem cache.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584443257
##########
File path: server/container/core/src/test/java/org/apache/james/server/core/MimeMessageWrapperTest.java
##########
@@ -256,15 +256,15 @@ public void testSizeModifiedHeaders() throws MessagingException {
public void testSizeModifiedBodyWithoutSave() throws MessagingException {
String newBody = "This is the new body of the message";
mw.setText(newBody);
- assertThat(mw.getSize()).isEqualTo(body.length());
+ assertThat(mw.getSize()).isEqualTo(-1);
}
@Test
public void testSizeModifiedBodyWithSave() throws MessagingException {
String newBody = "This is the new body of the message";
mw.setText(newBody);
mw.saveChanges();
- assertThat(mw.getSize()).isEqualTo(body.length());
+ assertThat(mw.getSize()).isEqualTo(-1);
Review comment:
I think as they don't tell anything about what behaviour is expected but just what part of what is tested...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584635692
##########
File path: mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
##########
@@ -439,9 +470,10 @@ private int getBodyStartOctet(BodyOffsetInputStream bIn) {
return bodyStartOctet;
}
- private AppendResult createAndDispatchMessage(Date internalDate, MailboxSession mailboxSession, File file, PropertyBuilder propertyBuilder, Flags flags, int bodyStartOctet) throws IOException, MailboxException {
- try (SharedFileInputStream contentIn = new SharedFileInputStream(file)) {
- final int size = (int) file.length();
+ private AppendResult createAndDispatchMessage(Date internalDate, MailboxSession mailboxSession, Content content, PropertyBuilder propertyBuilder, Flags flags, int bodyStartOctet) throws IOException, MailboxException {
+ // TODO MailboxMessage should be backed by a "content"
+ try (SharedFileInputStream contentIn = null) {
Review comment:
Yes. And it get fixed later. (I assume it).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584443105
##########
File path: server/container/core/src/main/java/org/apache/james/server/core/MimeMessageWrapper.java
##########
@@ -361,13 +361,8 @@ public int getSize() throws MessagingException {
throw new MessagingException("Unable to calculate message size");
}
} else {
- if (!messageParsed) {
- loadMessage();
- }
-
- return super.getSize();
+ return -1;
Review comment:
UNKNOWN
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584636012
##########
File path: mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageManager.java
##########
@@ -321,27 +321,17 @@ public AppendResult appendMessage(InputStream msgIn, Date internalDate, final Ma
// source for the InputStream
file = File.createTempFile("imap", ".msg");
try (FileOutputStream out = new FileOutputStream(file);
- BufferedOutputStream bufferedOut = new BufferedOutputStream(out);
Review comment:
No it is directly related to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] mbaechler commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
mbaechler commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r585558336
##########
File path: server/protocols/protocols-smtp/src/main/java/org/apache/james/smtpserver/DataLineJamesMessageHookHandler.java
##########
@@ -135,6 +134,18 @@ public Response onLine(SMTPSession session, ByteBuffer lineByteBuffer, LineHandl
return null;
}
+ private byte[] readBytes(ByteBuffer line) {
Review comment:
me neither
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on a change in pull request #309: Do not copy messages in memory before saving them to the blob store
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #309:
URL: https://github.com/apache/james-project/pull/309#discussion_r584630870
##########
File path: mailbox/store/src/test/java/org/apache/james/mailbox/store/mail/model/AttachmentMapperTest.java
##########
@@ -159,7 +160,7 @@ void getRelatedMessageIdsShouldReturnMessageIdWhenStoredWithMessageId() throws E
MessageId messageId = generateMessageId();
AttachmentId attachmentId = attachmentMapper.storeAttachmentsForMessage(ImmutableList.of(ParsedAttachment.builder()
.contentType("content")
- .content("".getBytes(StandardCharsets.UTF_8))
+ .content(ByteSource.wrap("".getBytes(StandardCharsets.UTF_8)))
Review comment:
But it works and is simple to understand / write.
You have an alternative?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org