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/10/27 09:27:46 UTC
[GitHub] [james-project] quantranhong1999 opened a new pull request, #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
quantranhong1999 opened a new pull request, #1276:
URL: https://github.com/apache/james-project/pull/1276
This is only a rare case, and the current code is resilient enough 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] quantranhong1999 closed pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
quantranhong1999 closed pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
URL: https://github.com/apache/james-project/pull/1276
--
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 pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1276:
URL: https://github.com/apache/james-project/pull/1276#issuecomment-1293326995
I would like to know `Why we have a message with a null date` in the first place...
--
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] quantranhong1999 commented on pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on PR #1276:
URL: https://github.com/apache/james-project/pull/1276#issuecomment-1293342593
> I would like to know Why we have a message with a null date in the first place...
https://github.com/apache/james-project/blob/master/upgrade-instructions.md#rework-message-denormalization
We spotted that we did finish that migration well for pre-prod during the Cassandra driver 4 upgradation.
--
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 pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1276:
URL: https://github.com/apache/james-project/pull/1276#issuecomment-1293651403
Even if not applied it is not supposed to matter to be honnest.
--
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 pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1276:
URL: https://github.com/apache/james-project/pull/1276#issuecomment-1293323109
SimpleMailboxMessage requires the internal date to be here. If it is missing -> error.
--
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] quantranhong1999 commented on a diff in pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on code in PR #1276:
URL: https://github.com/apache/james-project/pull/1276#discussion_r1006652097
##########
third-party/rspamd/src/main/java/org/apache/james/rspamd/task/GetMailboxMessagesService.java:
##########
@@ -109,8 +109,8 @@ private Flux<MessageResult> getMailboxMessagesOfAUser(Username username, Mailbox
return Mono.from(mailboxManager.getMailboxReactive(mailboxMetaData.getId(), mailboxSession))
.map(Throwing.function(MessageManager::getMailboxEntity))
.flatMapMany(Throwing.function(mailbox -> mapperFactory.getMessageMapper(mailboxSession).findInMailboxReactive(mailbox, MessageRange.all(), MessageMapper.FetchType.METADATA, UNLIMITED)))
- .doOnNext(mailboxMessageMetaData -> context.incrementHamMessageCount())
.filter(mailboxMessageMetaData -> afterDate.map(date -> mailboxMessageMetaData.getInternalDate().after(date)).orElse(true))
Review Comment:
As I understand, we always write not null `internalDate` to our DB. This is only null when migration mistake.
--
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 #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
vttranlina commented on code in PR #1276:
URL: https://github.com/apache/james-project/pull/1276#discussion_r1006649400
##########
third-party/rspamd/src/main/java/org/apache/james/rspamd/task/GetMailboxMessagesService.java:
##########
@@ -109,8 +109,8 @@ private Flux<MessageResult> getMailboxMessagesOfAUser(Username username, Mailbox
return Mono.from(mailboxManager.getMailboxReactive(mailboxMetaData.getId(), mailboxSession))
.map(Throwing.function(MessageManager::getMailboxEntity))
.flatMapMany(Throwing.function(mailbox -> mapperFactory.getMessageMapper(mailboxSession).findInMailboxReactive(mailbox, MessageRange.all(), MessageMapper.FetchType.METADATA, UNLIMITED)))
- .doOnNext(mailboxMessageMetaData -> context.incrementHamMessageCount())
.filter(mailboxMessageMetaData -> afterDate.map(date -> mailboxMessageMetaData.getInternalDate().after(date)).orElse(true))
Review Comment:
Q:
why we don't check null for case `mailboxMessageMetaData.getInternalDate()` ?
--
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 pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
chibenwa commented on PR #1276:
URL: https://github.com/apache/james-project/pull/1276#issuecomment-1293318498
Isn't it better to "JUST" run MessageDenormalizationMigration on the target envirnment?
Also, there is supposed to be fallback support in order to read the `internalDate` from `messagev3` when it is missing from either `imapUidTable` or `messageIdTable`. Such behavior is implemented in `CassandraMessageMapper` as well as `CassandraMessageIdMapper`.
This fix only scratches the surface of the issue and is unsatfying to me. We need to dig deeper.
--
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] quantranhong1999 commented on a diff in pull request #1276: JAMES-3775 Tests for null internalDate edge case (only when migration issue)
Posted by GitBox <gi...@apache.org>.
quantranhong1999 commented on code in PR #1276:
URL: https://github.com/apache/james-project/pull/1276#discussion_r1006684534
##########
third-party/rspamd/src/main/java/org/apache/james/rspamd/task/GetMailboxMessagesService.java:
##########
@@ -109,8 +109,8 @@ private Flux<MessageResult> getMailboxMessagesOfAUser(Username username, Mailbox
return Mono.from(mailboxManager.getMailboxReactive(mailboxMetaData.getId(), mailboxSession))
.map(Throwing.function(MessageManager::getMailboxEntity))
.flatMapMany(Throwing.function(mailbox -> mapperFactory.getMessageMapper(mailboxSession).findInMailboxReactive(mailbox, MessageRange.all(), MessageMapper.FetchType.METADATA, UNLIMITED)))
- .doOnNext(mailboxMessageMetaData -> context.incrementHamMessageCount())
.filter(mailboxMessageMetaData -> afterDate.map(date -> mailboxMessageMetaData.getInternalDate().after(date)).orElse(true))
Review Comment:
BTW I don't think we are accepting creating an object with null internal date (throw NPE like this) but predicate checking instead (throwing some IllegalArgumentException). Anyway, let's say RuntimeException generally and these tests are mainly proving what happened with a null internal date in the pre-prod Cassandra table.
--
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