Mailbox 285 Solve issues with flags update #556

chibenwa · 2017-01-11T07:31:45Z

Flags Updates on something else than the last message
Flags upates on duplicated mailboxIds

quynhn · 2017-01-11T07:44:51Z

👍

chibenwa · 2017-01-11T10:37:54Z

(tested on my laptop with 10 threads that does 100 updates each)

chibenwa · 2017-01-11T10:40:48Z

Concurrent test for MessageMapper and possible corrections needs to be done as well....

rouazana · 2017-01-11T16:25:32Z

...ssandra/src/main/java/org/apache/james/mailbox/cassandra/mail/MailboxDeleteDuringUpdate.java

+
+package org.apache.james.mailbox.cassandra.mail;
+
+public class MailboxDeleteDuringUpdate extends RuntimeException {


*MailboxDeleteDuringUpdateException

rouazana · 2017-01-11T16:26:53Z

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

+    }
+
+    @ContractTest
+    public void setFlagsShouldWorkWithConcurency() throws Exception {


*Concurrency

rouazana · 2017-01-11T16:28:10Z

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

+        List<MailboxMessage> messages = sut.find(ImmutableList.of(message1.getMessageId()), MessageMapper.FetchType.Body);
+        assertThat(messages).hasSize(1);
+        assertThat(messages.get(0).createFlags().getUserFlags()).hasSize(threadCount * updateCount);
+    }


Do you think you can add the same tests with some delete operations instead of add?

Won't it become highly correletated with execution order then ?

nope: first create the n flags, then delete them in whatever order and just ensure all are removed.

my idea was simpler: first you create the n flags at once, without threads. And then you delete all of them with threads.

rouazana · 2017-01-11T16:31:22Z

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

+                    messageMapper.updateFlags(mailbox,
+                        new FlagsUpdateCalculator(new Flags("custom-" + threadNumber + "-" + i),
+                            FlagsUpdateMode.ADD),
+                        MessageRange.one(uid));


Maybe you can extract all but this call in a lamba which could be a parameter to ConcurrentSetFlagTestRunnable and then make this one shared between the two test classes.

chibenwa · 2017-01-12T02:13:55Z

[Building Java 8 mailbox-285-c71270be-e3fc-460a-b646-a33bb97c6c35] Tests run: 41, Failures: 0, Errors: 2, Skipped: 1, Time elapsed: 32.403 sec <<< FAILURE! - in org.apache.james.jmap.cassandra.cucumber.CassandraUploadCucumberTest
[Building Java 8 mailbox-285-c71270be-e3fc-460a-b646-a33bb97c6c35] When "non-authenticated@domain.tld" upload a content(Scenario: Uploading a content without being authenticated)  Time elapsed: 0.006 sec  <<< ERROR!
[Building Java 8 mailbox-285-c71270be-e3fc-460a-b646-a33bb97c6c35] java.net.SocketException: Broken pipe
[Building Java 8 mailbox-285-c71270be-e3fc-460a-b646-a33bb97c6c35] 
[Building Java 8 mailbox-285-c71270be-e3fc-460a-b646-a33bb97c6c35] Scenario: Uploading a content without being authenticated  Time elapsed: 0.007 sec  <<< ERROR!
[Building Java 8 mailbox-285-c71270be-e3fc-460a-b646-a33bb97c6c35] java.net.SocketException: Broken pipe

chibenwa · 2017-01-12T02:14:06Z

test this please

chibenwa · 2017-01-12T02:52:04Z

CI CI CI !!!!

[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] [INFO] Apache James MPT ManageSieve Cassandra ............. FAILURE [ 17.057 s]

DeleteScriptTest.deleteScriptShouldWork:45->AbstractSimpleScriptedTestProtocol.scriptTest:78->AbstractProtocolTestFramework.runSessions:104 » InvalidServerResponse
[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] 
[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] Tests run: 14, Failures: 0, Errors: 1, Skipped: 0

[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] Location: /org/apache/james/managesieve/scripts/deletescript.test:76
[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] LastClientMsg: DELETESCRIPT "mysievescript"
[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] Expected: 'NO \(ACTIVE\) "You may not delete an active script"'
[Building Java 8 mailbox-285-a7b336e3-4e70-4803-bae1-2fcde5fb2cab] Actual   : 'OK'

chibenwa · 2017-01-12T02:52:11Z

test this please

chibenwa · 2017-01-12T02:55:03Z

@rouazana

no test for this commit?

Hmmm... I'm very sorry to say so but I have stricktly no idea on haw to write such tests without polluting implementation with something that would allow me to do barrier injection (what I don't want!)

chibenwa · 2017-01-13T03:32:07Z

Rebased on master

The only addition is the last commit. It introduced a mix of ADD and REMOVE.

(Note : to avoid REMOVE of a flag being sceduled before it is added, I need to ensure it is done by the same thread)

mbaechler · 2017-01-16T07:34:54Z

...assandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java

        return mailboxIds.stream()
-            .map(mailboxId -> (CassandraId) mailboxId)


I really prefer having the cast first

mbaechler · 2017-01-16T07:37:43Z

...assandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java

-            .map(composedMessageId -> flagsUpdateWithRetry(newState, updateMode, composedMessageId))
+            .distinct()
+            .filter(mailboxId -> imapUidDAO.retrieve((CassandraMessageId) messageId, Optional.of((CassandraId) mailboxId))
+                .join()


not related to this review but the join here break the "no early join" rule

Well if we want to get ride of it, we need our retrier to handle futures

mbaechler · 2017-01-16T07:40:12Z

...assandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java

+                .findAny()
+                .isPresent())
+            .map(mailboxId -> flagsUpdateWithRetry(newState, updateMode, mailboxId, messageId))
+            .filter(Optional::isPresent)


flagsUpdateWithRetry being a private method, it could return a Stream on zero or one element and flatmap here

Also mdified in CassandraMessageMapper

mbaechler · 2017-01-16T07:41:42Z

...assandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java

-                        newFlagsWithModSeq.getLeft()));
+            Pair<Flags, ComposedMessageIdWithMetaData> pair = new FunctionRunnerWithRetry(MAX_RETRY)
+                .executeAndRetrieveObject(() -> tryFlagsUpdate(newState, updateMode, mailboxId, messageId));
+            return Optional.of(Pair.of(pair.getRight().getComposedMessageId().getMailboxId(),


pair.getRight is called 4 times, I think it deserves a temp (named) variable

mbaechler · 2017-01-16T07:43:35Z

...assandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java

-        CompletableFuture<Boolean> imapUidFuture = imapUidDAO.updateMetadata(composedMessageIdWithMetaData, oldModSeq);
-        CompletableFuture<Boolean> messageIdFuture = messageIdDAO.updateMetadata(composedMessageIdWithMetaData, oldModSeq);
-        return imapUidFuture.join() && messageIdFuture.join();
+    private Optional<Pair<Flags, ComposedMessageIdWithMetaData>> updateFlags(MailboxId mailboxId, MessageId messageId, Flags newState, MessageManager.FlagsUpdateMode updateMode) throws MailboxException {


this method is too big

mbaechler · 2017-01-16T07:51:47Z

...rc/test/java/org/apache/james/mailbox/store/mail/model/concurrency/ConcurrentTestRunner.java

+            executorService.submit(new ConcurrentRunnableTask(i));
+        }
+        executorService.shutdown();
+        assertThat(executorService.awaitTermination(1, TimeUnit.MINUTES))


you should not use an assert here, you can throw instead

mbaechler · 2017-01-16T07:52:53Z

...rc/test/java/org/apache/james/mailbox/store/mail/model/concurrency/ConcurrentTestRunner.java

+            .isTrue();
+    }
+
+    protected abstract void performOperation(int threadNumber, int step) throws Exception;


composition ! please requires a BiConsumer in your runner, it makes things way simpler to use

mbaechler · 2017-01-16T07:53:50Z

...rc/test/java/org/apache/james/mailbox/store/mail/model/concurrency/ConcurrentTestRunner.java

+            executorService.submit(new ConcurrentRunnableTask(i));
+        }
+        executorService.shutdown();
+        assertThat(executorService.awaitTermination(1, TimeUnit.MINUTES))


I would add that you should allow the caller to do that await, for example by returning a CompletableFuture.

No method to do that by completableFuture, but behaviour added

mbaechler · 2017-01-16T07:54:18Z

...rc/test/java/org/apache/james/mailbox/store/mail/model/concurrency/ConcurrentTestRunner.java

+
+    public void run() throws Exception {
+        ExecutorService executorService = Executors.newFixedThreadPool(threadCount);
+        for (int i = 0; i < threadCount; i++) {


what about a java8 stream with range ? (:

It's not in a java8 project

mbaechler · 2017-01-16T07:54:52Z

...rc/test/java/org/apache/james/mailbox/store/mail/model/concurrency/ConcurrentTestRunner.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public abstract class ConcurrentTestRunner {


what about some tests ?

Modseq used for condition was not well retrieved

…eIdMapper

…pdates

…tFlags

chibenwa · 2017-01-17T04:34:38Z

test this please

chibenwa · 2017-01-17T07:49:35Z

test this please

rouazana · 2017-01-17T10:24:54Z

...assandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java

+    private Optional<Pair<Flags, ComposedMessageIdWithMetaData>> updateFlags(ComposedMessageIdWithMetaData oldComposedId, ComposedMessageIdWithMetaData newComposedId) {
+        return imapUidDAO.updateMetadata(newComposedId, oldComposedId.getModSeq())
+            .thenCompose(updateSuccess -> Optional.of(updateSuccess)
+                .filter(b -> b)


what are these filters?

To repkace if as asked by Matthieu

rouazana · 2017-01-17T10:27:04Z

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

+        List<MailboxMessage> messages = sut.find(ImmutableList.of(message1.getMessageId()), MessageMapper.FetchType.Body);
+        assertThat(messages).hasSize(1);
+        assertThat(messages.get(0).createFlags().getUserFlags()).hasSize(threadCount * updateCount);
+    }


my idea was simpler: first you create the n flags at once, without threads. And then you delete all of them with threads.

chibenwa · 2017-01-17T16:44:58Z

Here at least it even mixes add and deletes

chibenwa · 2017-01-17T16:50:13Z

test this please

ddolcimascolo approved these changes Jan 11, 2017

View reviewed changes

aduprat approved these changes Jan 11, 2017

View reviewed changes

chibenwa closed this Jan 11, 2017

chibenwa reopened this Jan 11, 2017

chibenwa force-pushed the MAILBOX-285 branch 2 times, most recently from 395436e to 06cf4e5 Compare January 11, 2017 11:32

rouazana reviewed Jan 11, 2017

View reviewed changes

chibenwa mentioned this pull request Jan 12, 2017

Test stability statistics #534

Closed

chibenwa force-pushed the MAILBOX-285 branch from 80c5782 to f4f3251 Compare January 13, 2017 03:30

aduprat approved these changes Jan 13, 2017

View reviewed changes

mbaechler suggested changes Jan 16, 2017

View reviewed changes

chibenwa added 8 commits January 17, 2017 10:44

MAILBOX-285 Correct bug on flags update

0944974

Modseq used for condition was not well retrieved

MAILBOX-285 Allow duplicates in mailboxId argument of CassandraMessag…

040c1b5

…eIdMapper

MAILBOX-285 Concurrent safe version for MessageIdMapper :: setFlags

f8eb73e

MAILBOX-285 Cassandra MessageMapper should support concurrent flags u…

16bba83

…pdates

MAILBOX-285 Allow message removal during CassandraMessageMapper :: se…

1bd6c7c

…tFlags

MAILBOX-285 Factorize concurrency testing

04b1580

MAILBOX-285 Adding concurrent flags update tests also for REMOVE

853f5c9

MAILBOX-285 Improve code quality for setFlags and related tests

14be7a1

chibenwa force-pushed the MAILBOX-285 branch from f4f3251 to 14be7a1 Compare January 17, 2017 03:59

rouazana reviewed Jan 17, 2017

View reviewed changes

rouazana approved these changes Jan 17, 2017

View reviewed changes

aduprat merged commit 14be7a1 into linagora:master Jan 18, 2017

rouazana mentioned this pull request Jan 18, 2017

Can no more change read/unread, star/unstar status in OpenPaaS ESN Inbox emails using James #554

Closed


		package org.apache.james.mailbox.cassandra.mail;

		public class MailboxDeleteDuringUpdate extends RuntimeException {

		return mailboxIds.stream()
		.map(mailboxId -> (CassandraId) mailboxId)

Mailbox 285 Solve issues with flags update #556

Mailbox 285 Solve issues with flags update #556

Conversation

chibenwa commented Jan 11, 2017

quynhn commented Jan 11, 2017

chibenwa commented Jan 11, 2017

chibenwa commented Jan 11, 2017

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 Jan 12, 2017

chibenwa commented Jan 12, 2017

chibenwa commented Jan 12, 2017

chibenwa commented Jan 12, 2017

chibenwa commented Jan 12, 2017

chibenwa commented Jan 13, 2017

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

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 Jan 17, 2017

chibenwa commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Jan 17, 2017

chibenwa commented Jan 17, 2017