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 2022/09/13 03:28:24 UTC

[GitHub] [james-project] vttranlina opened a new pull request, #1189: JAMES-3775 Refactor GetMailboxMessagesService

vttranlina opened a new pull request, #1189:
URL: https://github.com/apache/james-project/pull/1189

   - Avoid collectList
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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] vttranlina commented on a diff in pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService

Posted by GitBox <gi...@apache.org>.
vttranlina commented on code in PR #1189:
URL: https://github.com/apache/james-project/pull/1189#discussion_r969170200


##########
third-party/rspamd/src/main/java/org/apache/james/rspamd/task/GetMailboxMessagesService.java:
##########
@@ -90,9 +90,7 @@ private Flux<MessageResult> getMailboxMessagesOfAUser(Username username, String
             .doOnNext(mailboxMessageMetaData -> context.incrementSpamMessageCount())
             .filter(mailboxMessageMetaData -> afterDate.map(date -> mailboxMessageMetaData.getInternalDate().after(date)).orElse(true))
             .filter(message -> randomBooleanWithProbability(runningOptions))
-            .map(Message::getMessageId)
-            .collectList()
-            .flatMapMany(messageIds -> messageIdManager.getMessagesReactive(messageIds, FetchGroup.FULL_CONTENT, mailboxSession))
+            .flatMap(message -> messageIdManager.getMessagesReactive(List.of(message.getMessageId()), FetchGroup.FULL_CONTENT, mailboxSession))

Review Comment:
   "window" is expensive, should we still consider in this case?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 diff in pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService + Clean code + Fix webadmin task

Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1189:
URL: https://github.com/apache/james-project/pull/1189#discussion_r969411151


##########
third-party/rspamd/src/test/java/org/apache/james/rspamd/client/RspamdHttpClientTest.java:
##########
@@ -121,11 +141,11 @@ void checkSpamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAs
     }
 
     @Test
-    void checkHamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAsSameAsUsingRawClient() {
+    void checkHamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAsSameAsUsingRawClient() throws MessagingException {

Review Comment:
   throws Exception



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 #1189: JAMES-3775 Refactor GetMailboxMessagesService + Clean code + Fix webadmin task

Posted by GitBox <gi...@apache.org>.
Arsnael commented on PR #1189:
URL: https://github.com/apache/james-project/pull/1189#issuecomment-1250539490

   And needs a rebase


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 diff in pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService

Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1189:
URL: https://github.com/apache/james-project/pull/1189#discussion_r969225767


##########
third-party/rspamd/src/main/java/org/apache/james/rspamd/task/GetMailboxMessagesService.java:
##########
@@ -90,9 +90,7 @@ private Flux<MessageResult> getMailboxMessagesOfAUser(Username username, String
             .doOnNext(mailboxMessageMetaData -> context.incrementSpamMessageCount())
             .filter(mailboxMessageMetaData -> afterDate.map(date -> mailboxMessageMetaData.getInternalDate().after(date)).orElse(true))
             .filter(message -> randomBooleanWithProbability(runningOptions))
-            .map(Message::getMessageId)
-            .collectList()
-            .flatMapMany(messageIds -> messageIdManager.getMessagesReactive(messageIds, FetchGroup.FULL_CONTENT, mailboxSession))
+            .flatMap(message -> messageIdManager.getMessagesReactive(List.of(message.getMessageId()), FetchGroup.FULL_CONTENT, mailboxSession))

Review Comment:
   No window by item count is cheap, but likely not necessary perf wise. Let's go ahead imo



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 merged pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService + Clean code + Fix webadmin task

Posted by GitBox <gi...@apache.org>.
Arsnael merged PR #1189:
URL: https://github.com/apache/james-project/pull/1189


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 diff in pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService + Clean code + Fix webadmin task

Posted by GitBox <gi...@apache.org>.
Arsnael commented on code in PR #1189:
URL: https://github.com/apache/james-project/pull/1189#discussion_r973825138


##########
third-party/rspamd/src/test/java/org/apache/james/rspamd/task/FeedHamToRspamdTaskTest.java:
##########
@@ -142,6 +165,22 @@ void taskShouldReportAllHamMessagesOfAllUsersByDefault() throws MailboxException
                 .build());
     }
 
+    @Test
+    void taskShouldHitToRspamdServerWhenLearnHam() throws MailboxException{

Review Comment:
   ```suggestion
       void taskShouldHitToRspamdServerWhenLearnHam() throws MailboxException {
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 diff in pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService

Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1189:
URL: https://github.com/apache/james-project/pull/1189#discussion_r969169507


##########
third-party/rspamd/src/main/java/org/apache/james/rspamd/task/GetMailboxMessagesService.java:
##########
@@ -90,9 +90,7 @@ private Flux<MessageResult> getMailboxMessagesOfAUser(Username username, String
             .doOnNext(mailboxMessageMetaData -> context.incrementSpamMessageCount())
             .filter(mailboxMessageMetaData -> afterDate.map(date -> mailboxMessageMetaData.getInternalDate().after(date)).orElse(true))
             .filter(message -> randomBooleanWithProbability(runningOptions))
-            .map(Message::getMessageId)
-            .collectList()
-            .flatMapMany(messageIds -> messageIdManager.getMessagesReactive(messageIds, FetchGroup.FULL_CONTENT, mailboxSession))
+            .flatMap(message -> messageIdManager.getMessagesReactive(List.of(message.getMessageId()), FetchGroup.FULL_CONTENT, mailboxSession))

Review Comment:
   We likely can use window operator to still read msgs by batch without reading them all



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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 diff in pull request #1189: JAMES-3775 Refactor GetMailboxMessagesService + Clean code + Fix webadmin task

Posted by GitBox <gi...@apache.org>.
chibenwa commented on code in PR #1189:
URL: https://github.com/apache/james-project/pull/1189#discussion_r969410344


##########
third-party/rspamd/src/test/java/org/apache/james/rspamd/client/RspamdHttpClientTest.java:
##########
@@ -95,24 +115,24 @@ void learnHamWithWrongPasswordShouldThrowUnauthorizedExceptionException() {
         RspamdClientConfiguration configuration = new RspamdClientConfiguration(rspamdExtension.getBaseUrl(), "wrongPassword", Optional.empty());
         RspamdHttpClient client = new RspamdHttpClient(configuration);
 
-        assertThatThrownBy(() -> reportAsHam(client, new ByteArrayInputStream(spamMessage)))
+        assertThatThrownBy(() -> reportAsHam(client, spamMessage.getMessage().getInputStream()))
             .hasMessage("{\"error\":\"Unauthorized\"}")
             .isInstanceOf(UnauthorizedException.class);
     }
 
     @Test
-    void checkSpamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAsSameAsUsingRawClient() {
+    void checkSpamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAsSameAsUsingRawClient() throws MessagingException {
         RspamdClientConfiguration configuration = new RspamdClientConfiguration(rspamdExtension.getBaseUrl(), PASSWORD, Optional.empty());
         RspamdHttpClient client = new RspamdHttpClient(configuration);
 
-        AnalysisResult analysisResult = client.checkV2(new ByteArrayInputStream(spamMessage)).block();
+        AnalysisResult analysisResult = client.checkV2(spamMessage).block();
         assertThat(analysisResult.getAction()).isEqualTo(AnalysisResult.Action.REJECT);
 
         RequestSpecification rspamdApi = WebAdminUtils.spec(Port.of(rspamdExtension.dockerRspamd().getPort()));
         rspamdApi
             .header(new Header("Password", PASSWORD))
             .body(ClassLoader.getSystemResourceAsStream(SPAM_MESSAGE_PATH))
-            .post("checkv2")
+            .post("checkv2").prettyPeek()

Review Comment:
   pretty peek



##########
third-party/rspamd/src/test/java/org/apache/james/rspamd/client/RspamdHttpClientTest.java:
##########
@@ -95,24 +115,24 @@ void learnHamWithWrongPasswordShouldThrowUnauthorizedExceptionException() {
         RspamdClientConfiguration configuration = new RspamdClientConfiguration(rspamdExtension.getBaseUrl(), "wrongPassword", Optional.empty());
         RspamdHttpClient client = new RspamdHttpClient(configuration);
 
-        assertThatThrownBy(() -> reportAsHam(client, new ByteArrayInputStream(spamMessage)))
+        assertThatThrownBy(() -> reportAsHam(client, spamMessage.getMessage().getInputStream()))
             .hasMessage("{\"error\":\"Unauthorized\"}")
             .isInstanceOf(UnauthorizedException.class);
     }
 
     @Test
-    void checkSpamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAsSameAsUsingRawClient() {
+    void checkSpamMailUsingRspamdClientWithExactPasswordShouldReturnAnalysisResultAsSameAsUsingRawClient() throws MessagingException {

Review Comment:
   throws Exception



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

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