You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "quantranhong1999 (via GitHub)" <gi...@apache.org> on 2025/12/10 09:43:31 UTC
[PR] JAMES-4154 Refactor message content deletion [james-project]
quantranhong1999 opened a new pull request, #2884:
URL: https://github.com/apache/james-project/pull/2884
cf https://issues.apache.org/jira/browse/JAMES-4154
Drop the RabbitMQ code for the deleted message vault callback. Rely on EventBus on this instead, and allow other listener(s) to handle the message content event easily.
This PR mostly focuses on the Distributed app. We will plan to contribute this to the Postgres app 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.
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2613700074
##########
src/adr/0073-dedicated-eventbus-for-message-content-deletion.md:
##########
@@ -0,0 +1,44 @@
+# 73. Relying on EventBus for message deletion side effects
+
+Date: 2025-12-12
+
+## Status
+
+Accepted (lazy consensus) & implemented.
+
+## Context
+
Review Comment:
A good hold friend told me we shall start ADRs specking of email. And that too technical ADRs shall be linked to functional concepts.
Let me try it:
```
James want to perform actions upon email deletion, for instance copying events in the Deleted Message Vault or clearing the corresponding message preview (used to serve an optimized data projection over the JMAP protocol).
Deletion action can be long (content copy for the [deleted message vault](TO BE WRITTEN)) and are thus in Cassandra and Postgress done asynchronously in the [DeleteMessageListener](https://github.com/apache/james-project/blob/master/src/adr/0029-Cassandra-mailbox-deletion-cleanup.md).
We encountered issues, most notably with mailbox deletion, wich triggered those operation on a message set of message, leading to event processing stalling and eventually timeout.
This had been historically solved in the Distributed server by adding a custom RabbitMQ queue for the deleted message vauklt to sequence and distribute each single deletions. While it lead to a viable use in production, this approach suffers from the following pitfall:
- It duplicates the Event bus code, used for work queues
- It requires a lot of custom code for adoption in other implementations
- It makes it hard to add other "features" in a composablee fashion without duplicating a lot of code
```
And while we are at it we could retrospectively write an ADR on the Deleted Message Vault (separated issue?)
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2613700074
##########
src/adr/0073-dedicated-eventbus-for-message-content-deletion.md:
##########
@@ -0,0 +1,44 @@
+# 73. Relying on EventBus for message deletion side effects
+
+Date: 2025-12-12
+
+## Status
+
+Accepted (lazy consensus) & implemented.
+
+## Context
+
Review Comment:
A good hold friend told me we shall start ADRs specking of email. And that too technical ADRs shall be linked to functional concepts.
Let me try it:
```
We want to perform actions upon email deletion, for instance copying events in the [Deleted Message Vault](TO BE WRITTEN) or clearing the corresponding message preview (used to serve an optimized data projection over the JMAP protocol).
Deletion action can be long (content copy) and are thus in Cassandra and Postgress done asynchronously in the [DeleteMessageListener](https://github.com/apache/james-project/blob/master/src/adr/0029-Cassandra-mailbox-deletion-cleanup.md).
We encountered issues, most notably with mailbox deletion, wich triggered those operation on a message set of message, leading to event processing stalling and eventually timeout.
This had been historically solved in the Distributed server by adding a custom RabbitMQ queue for the deleted message vauklt to sequence and distribute each single deletions. While it lead to a viable use in production, this approach suffers from the following pitfall:
- It duplicates the Event bus code, used for work queues
- It requires a lot of custom code for adoption in other implementations
- It makes it hard to add other "features" in a composablee fashion without duplicating a lot of code
```
And while we are at it we could retrospectively write an ADR on the Deleted Message Vault (separated issue?)
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609068182
##########
mailbox/api/src/main/java/org/apache/james/mailbox/events/MailboxEvents.java:
##########
@@ -536,6 +536,24 @@ public final int hashCode() {
}
}
+ record MessageContentDeletionEvent(EventId eventId, Username username, MailboxId mailboxId, MessageId messageId, long size,
+ Instant internalDate, boolean hasAttachments, String headerBlobId, String bodyBlobId) implements Event {
+
+ @Override
+ public EventId getEventId() {
+ return eventId;
+ }
+
+ public Username getUsername() {
+ return username;
Review Comment:
Unnecessary getter?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on PR #2884:
URL: https://github.com/apache/james-project/pull/2884#issuecomment-3652643044
> > Seems no blob module for the JPA app. Should we revert the strong type commit for BlobId, or bind a PlainBlobId factory for it cc @chibenwa ?
>
> I would say keeping the strong type is better here, binding a PlainBlobId factory for jpa should be alright?
IMO, we should not. Forcing the "blob" concept into the JPA app where it does not know or does not use seems wrong to me.
I think I will revert the strong type commit and let this PR move forward. If needed, we can comeback on this later.
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2608915556
##########
mailbox/api/src/main/java/org/apache/james/mailbox/events/MailboxEvents.java:
##########
@@ -536,6 +536,101 @@ public final int hashCode() {
}
}
+ class MessageContentDeletionEvent implements Event {
Review Comment:
record maybe?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609601923
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
Or even...
Just keeping the current implementation.
But we fully remove deletion callbacks.
Rational:
- unsafe to use
- relevant implems use a safer listener based implementation
- drop one interface
- no longer manual plugging into MessageManager
This would be a major code simplification that definitely looks worth it.
Tldr: I would like us to explore this refactoring further!
Delete listener just ALWAYS publish to the event bus (with aforementioned optim) and we drop deletion callbacks.
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #2884:
URL: https://github.com/apache/james-project/pull/2884#issuecomment-3639774088
Related? https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2884/1/testReport/
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609405998
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
> Event bus callback would then likely be reused as is for the PG codepath
Different `DeletionCallback` interface between Cassandra and Postgres implementations. So it is not a drop in reusage.
I think the question should be more: Do we want to always bind the content deletion event bus, regardless of vault enabled?
If we use the content deletion event bus directly in `DeleteMessageListener`, we always bind the content deletion event bus (hence, the event bus work queue is created, potentially without any real usage).
If we implement the dispatch as a callback, potentially we can plug it only when the vault work queue is enabled? So we can use the event bus work queue only when needed. However, if other listeners (e.g. `RagDeletionListener`) want to subscribe to the content deletion event bus, it is not easy as it depends on the vault usage.
I am not sure what the best way to go is. Maybe for James, we only bind the content deletion event bus when vault work queue is enabled. For Tmail, we always bind the content deletion event bus to ease the usage?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609310366
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
I would have implemented the dispatch as a callback, which would fully avoid eventbus usage in this class,
Event bus callback would then likely be reused as is for the PG codepath
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #2884:
URL: https://github.com/apache/james-project/pull/2884#issuecomment-3652630019
> Seems no blob module for the JPA app. Should we revert the strong type commit for BlobId, or bind a PlainBlobId factory for it cc @chibenwa ?
I would say keeping the strong type is better here, binding a PlainBlobId factory for jpa should be alright?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609990376
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
Dropped the deletion callback (for Cassandra).
For Postgres, we can plan to refactor in https://github.com/linagora/tmail-backend/issues/2032.
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609670578
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
Interesting suggestion. Let's see how it goes.
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609074497
##########
mailbox/api/src/main/java/org/apache/james/mailbox/events/MailboxEvents.java:
##########
@@ -536,6 +536,24 @@ public final int hashCode() {
}
}
+ record MessageContentDeletionEvent(EventId eventId, Username username, MailboxId mailboxId, MessageId messageId, long size,
+ Instant internalDate, boolean hasAttachments, String headerBlobId, String bodyBlobId) implements Event {
+
+ @Override
+ public EventId getEventId() {
+ return eventId;
+ }
+
+ public Username getUsername() {
+ return username;
Review Comment:
No, it was needed.
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 merged PR #2884:
URL: https://github.com/apache/james-project/pull/2884
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609300284
##########
mailbox/api/src/main/java/org/apache/james/mailbox/events/MailboxEvents.java:
##########
@@ -536,6 +536,25 @@ public final int hashCode() {
}
}
+ record MessageContentDeletionEvent(EventId eventId, Username username, MailboxId mailboxId, MessageId messageId, long size,
+ Instant internalDate, boolean hasAttachments, String headerBlobId, String bodyBlobId) implements Event {
Review Comment:
```suggestion
Instant internalDate, boolean hasAttachments, BlobId headerBlobId, BlobId bodyBlobId) implements Event {
```
?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on PR #2884:
URL: https://github.com/apache/james-project/pull/2884#issuecomment-3650737881
```java
1) [Guice/MissingImplementation]: No implementation for BlobId$Factory was bound.
Requested by:
1 : MailboxEventSerializer.<init>(MailboxEventSerializer.scala:456)
\_ for 3rd parameter blobIdFactory
at DefaultEventModule.configure(DefaultEventModule.java:52)
```
Seems no blob module for the JPA app. Should we revert the strong type commit for `BlobId`, or bind a PlainBlobId factory for it cc @chibenwa ?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609457920
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
> Different DeletionCallback interface between Cassandra and Postgres implementations.
IMO we can factorize it in mailbox-store?
> Do we want to always bind the content deletion event bus, regardless of vault enabled?
IMO yes
I would however propose an optimisation on Rabbit group event bus side: publis if and only if we have group listener regitered. However if no group listeners are registered we can skip publishing in the first place.
That way we avoid needless event publication without asking ourselves if we need or not the eventbus.
Thoughts?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609532816
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
> I would however propose an optimisation on Rabbit group event bus side: publis if and only if we have group listener regitered. However if no group listeners are registered we can skip publishing in the first place.
We needed to register a noop listener on the content deletion listener. Otherwise, the queue won't get created until there is a listener registered, and the consumer healthcheck would fail (queue not found). That is kinda against the idea you proposed...
Proposal:
- Always guice binding for the content deletion event bus
- Implement the messageContentDeleted dispatch as a callback e.g. `DispatchMessageContentDeletionCallback`, and in James, bind it only when the vault work queue is enabled.
So we do not waste time deserializing and dispatching the event when we do not need it.
- In TMail, in the `AiBotModule`, we can bind the `DispatchMessageContentDeletionCallback` + `RagDeletionListener`. Hopefully, the `DispatchMessageContentDeletionCallback` can override the existing binding, if any, from the vault module.
That way, we can have the dispatch work only when needed, regardless of whether the vault or rag is enabled.
Thoughts?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609601923
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/DeleteMessageListener.java:
##########
@@ -270,11 +283,29 @@ private Mono<Void> handleMessageDeletion(CassandraMessageId messageId, MailboxId
.then(threadLookupDAO.deleteOneRow(threadId, messageId)));
}
+ private Mono<Void> dispatchMessageContentDeletionEvent(MailboxId mailboxId, Username owner, MessageRepresentation message) {
+ return Mono.from(contentDeletionEventBus.dispatch(EventFactory.messageContentDeleted()
+ .randomEventId()
+ .user(owner)
+ .mailboxId(mailboxId)
+ .messageId(message.getMessageId())
+ .size(message.getSize())
+ .instant(message.getInternalDate().toInstant())
+ .hasAttachments(!message.getAttachments().isEmpty())
+ .headerBlobId(message.getHeaderId().asString())
+ .bodyBlobId(message.getBodyId().asString())
+ .build(),
+ ImmutableSet.of()));
+ }
+
private Mono<Void> handleMessageDeletionAsPartOfMailboxDeletion(CassandraMessageId messageId, ThreadId threadId, CassandraId excludedId, Username owner) {
return Mono.just(messageId)
.filterWhen(id -> isReferenced(id, excludedId))
.flatMap(id -> readMessage(id)
- .flatMap(message -> Flux.fromIterable(deletionCallbackList).concatMap(callback -> callback.forMessage(message, excludedId, owner)).then().thenReturn(message))
+ .flatMap(message -> Flux.fromIterable(deletionCallbackList)
+ .concatMap(callback -> callback.forMessage(message, excludedId, owner))
+ .then(dispatchMessageContentDeletionEvent(excludedId, owner, message)
Review Comment:
Or even...
Just keeping the current implementation.
But we fully remove deletion callbacks.
Rational:
- unsafe to use
- relevant implems use a safer listener based implementation
- drop one interface
- no longer manual plugging into MessageManager
This would be a major code simplification that definitely looks worth it.
Tldr: I would like us to explore this refactoring further!
Delete listener just ALWAYS publish to the event bus (with aforementioned optim) and we drop deletion callbacks.
We can share the publishing code in a helper for reuse in pg layer.
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2884:
URL: https://github.com/apache/james-project/pull/2884#issuecomment-3640753733
The event serializer is in the event bus layer. It has enough info to serialize if and only if there is listeners.
So why making the functional layer more complex? Why does it need to care?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "quantranhong1999 (via GitHub)" <gi...@apache.org>.
quantranhong1999 commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609307297
##########
mailbox/api/src/main/java/org/apache/james/mailbox/events/MailboxEvents.java:
##########
@@ -536,6 +536,25 @@ public final int hashCode() {
}
}
+ record MessageContentDeletionEvent(EventId eventId, Username username, MailboxId mailboxId, MessageId messageId, long size,
+ Instant internalDate, boolean hasAttachments, String headerBlobId, String bodyBlobId) implements Event {
Review Comment:
Is it ok to add the `blob-api` dependency in the `mailbox-api` module?
--
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
Re: [PR] JAMES-4154 Refactor message content deletion [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2884:
URL: https://github.com/apache/james-project/pull/2884#discussion_r2609320916
##########
mailbox/api/src/main/java/org/apache/james/mailbox/events/MailboxEvents.java:
##########
@@ -536,6 +536,25 @@ public final int hashCode() {
}
}
+ record MessageContentDeletionEvent(EventId eventId, Username username, MailboxId mailboxId, MessageId messageId, long size,
+ Instant internalDate, boolean hasAttachments, String headerBlobId, String bodyBlobId) implements Event {
Review Comment:
Seen that it leads to complex injection of the blobIdFactory: not a hard blocker 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.
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