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/01/07 16:31:13 UTC
[GitHub] [james-project] chibenwa opened a new pull request #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
chibenwa opened a new pull request #831:
URL: https://github.com/apache/james-project/pull/831
- The Pulsar mail queue currently do not delete delayed emails
Explanation: the sequenceId of the mail is reset when
moved from the scheduled topic to the out topic, thus
bypassing deletes emitted while it was sitting in the
scheduled queue.
(this mechanism exist to prevent deletions to apply on
future emails)
We would likely need the sequence to be backed by some
applicative metadata - like a timestamp?
- Second problem: filter removal based on sequence number
Goal: Prevent uncontrolled growth by removing
meaningless deletion filters
Assumptions: emails are ordered
Pitfall: delayed messages are out of order.
Please note that the JMS mailqueue is buggy regarding
delayed messages being deleted, I used the memory implementation as a reference.
--
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] jeantil commented on pull request #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007911804
Before we undertake aligning the behavior with the updated contract, can you confirm the following :
> Removal should apply on all items enqueued before remove is called regardless of whether they are scheduled in the future or not. Removal should not apply on any items that are enqueued after the call to remove.
in other words, removal applies on enqueue order and not on dequeue order.
Assuming we implement the 2nd option I proposed : duplicate the filter logic for items consumed from the scheduled topic before they are injected in the out topic using the schedule topic offset at the time of remove as a reference for filtering, do you still see
> Second problem: filter removal based on sequence number
as a fundamental problem that needs to be solved ? I can't think of anything but I could be missing something.
--
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 edited a comment on pull request #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007882993
> I suggest not starting a third discussion on the same topic.
Fair. But failing tests have added value and shows fonctionnal expectations I have.
It also makes the whole discussion less abstract (and show I care enough to dedicate a bit of time to the topic).
I do not feel like this just is valueless conversation duplication.
BTW it proved we have two distinct problems while the design review only allowed to catch one!
> so we implemented the simplest that passed the contracts :)
I have limited faight in these contracts, delays is not an area covered by the previous RabbitMQ implementation and we did devote limited amount of time testing edge cases, interactions with other MailQueue features. It should not be taken as "written in marble".
> attach a monotonously increasing identifier such as a timestamp
:+1:
That's what I meant by "We would likely need the sequence to be backed by some applicative metadata - like a timestamp?". Of course it brings additional problems like accounting for imperfect clock synchronisation with a tolerated clock skew.
> query the scheduled topic offset on filter creation and attach that to the filter in addition to the out topic offset. filter scheduled messages as they are consumed and before they are requeued in the out topic. a filter would then expire when both the out topic and the scheduled topic's offset have been exceeded. (a simple way to implement this with the current setup is to duplicate the filter actor and the filter command queue and reuse the existing logic for the scheduled queue)
:+1:
--
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] jeantil commented on pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1010041573
you will need to @Disable them in the PulsarMailqueueTest, we will enable them again when we merge (adding the @Disabled will ensure we get a conflict when rebasing :)
--
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] jeantil commented on pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1010568448
My bad. I shouldn't have tried to do the review while managing the kids.
Le mar. 11 janv. 2022 à 16:24, Benoit TELLIER ***@***.***> a
écrit :
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
> server/queue/queue-pulsar/src/test/java/org/apache/james/queue/pulsar/PulsarMailQueueTest.java
> <https://github.com/apache/james-project/pull/831#discussion_r782254109>:
>
> > + @Test
> + @Override
> + @Disabled("JAMES-3687 Delayed deletes are buggy")
> + public void delayedEmailsShouldBeDeleted() {
> +
> + }
> +
> + @Test
> + @Override
> + @Disabled("JAMES-3687 Delayed deletes are buggy")
> + public void delayedEmailsShouldBeDeletedWhenMixedWithOtherEmails() {
> +
> + }
>
> you will need to @disable <https://github.com/disable> them in the
> PulsarMailqueueTest, we will enable them again when we merge (adding the
> @disabled <https://github.com/disabled> will ensure we get a conflict
> when rebasing :)
>
> Isn't this done here?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/james-project/pull/831#pullrequestreview-849334374>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAFTQ3ADH7SEQJP6HIIYO3UVRDSDANCNFSM5LPEZ2PA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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 change in pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #831:
URL: https://github.com/apache/james-project/pull/831#discussion_r782682458
##########
File path: server/queue/queue-api/src/test/java/org/apache/james/queue/api/DelayedManageableMailQueueContract.java
##########
@@ -70,6 +72,61 @@ default void delayedMessagesShouldBeCleared() throws Exception {
assertThat(getManageableMailQueue().getSize()).isEqualTo(0L);
}
+ @Test
+ default void delayedEmailsShouldBeDeleted() throws Exception {
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("abc")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+ // The queue being FIFO a second email can serve as a wait condition
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("def")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ getManageableMailQueue().remove(ManageableMailQueue.Type.Name, "abc");
+
+ ArrayList<String> names = new ArrayList<>();
+ Flux.from(getManageableMailQueue().deQueue())
+ .subscribeOn(Schedulers.elastic())
+ .subscribe(item -> names.add(item.getMail().getName()));
+
+ Awaitility.await().untilAsserted(() -> assertThat(names).contains("def"));
+ assertThat(names).containsExactly("def");
+ }
+
+ @Test
+ default void delayedEmailsShouldBeDeletedWhenMixedWithOtherEmails() throws Exception {
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("abc")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ getManageableMailQueue().remove(ManageableMailQueue.Type.Name, "abc");
+
+ // The newer email
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("def")
+ .build());
+ // The queue being FIFO a third email can serve as a wait condition
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("ghi")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ ArrayList<String> names = new ArrayList<>();
+ Flux.from(getManageableMailQueue().deQueue())
+ .subscribeOn(Schedulers.elastic())
+ .subscribe(item -> names.add(item.getMail().getName()));
+
+ Awaitility.await().untilAsserted(() -> assertThat(names).contains("ghi"));
Review comment:
indent
##########
File path: server/queue/queue-api/src/test/java/org/apache/james/queue/api/DelayedManageableMailQueueContract.java
##########
@@ -70,6 +72,61 @@ default void delayedMessagesShouldBeCleared() throws Exception {
assertThat(getManageableMailQueue().getSize()).isEqualTo(0L);
}
+ @Test
+ default void delayedEmailsShouldBeDeleted() throws Exception {
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("abc")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+ // The queue being FIFO a second email can serve as a wait condition
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("def")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ getManageableMailQueue().remove(ManageableMailQueue.Type.Name, "abc");
+
+ ArrayList<String> names = new ArrayList<>();
+ Flux.from(getManageableMailQueue().deQueue())
+ .subscribeOn(Schedulers.elastic())
+ .subscribe(item -> names.add(item.getMail().getName()));
+
+ Awaitility.await().untilAsserted(() -> assertThat(names).contains("def"));
Review comment:
indent
--
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 change in pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #831:
URL: https://github.com/apache/james-project/pull/831#discussion_r782254109
##########
File path: server/queue/queue-pulsar/src/test/java/org/apache/james/queue/pulsar/PulsarMailQueueTest.java
##########
@@ -238,4 +238,18 @@ private void assertThatStoreIsEmpty() {
@Override
public void flushShouldPreserveBrowseOrder() {
}
+
+ @Test
+ @Override
+ @Disabled("JAMES-3687 Delayed deletes are buggy")
+ public void delayedEmailsShouldBeDeleted() {
+
+ }
+
+ @Test
+ @Override
+ @Disabled("JAMES-3687 Delayed deletes are buggy")
+ public void delayedEmailsShouldBeDeletedWhenMixedWithOtherEmails() {
+
+ }
Review comment:
> you will need to @disable them in the PulsarMailqueueTest, we will enable them again when we merge (adding the
> @disabled will ensure we get a conflict when rebasing :)
Isn't this done 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.
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 #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007547937
--
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 #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1009540344
As these are just tests I think we can merge them to the contract prior to the fix.
I opened a separate issue https://issues.apache.org/jira/browse/JAMES-3698
--
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 change in pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #831:
URL: https://github.com/apache/james-project/pull/831#discussion_r782740126
##########
File path: server/queue/queue-api/src/test/java/org/apache/james/queue/api/DelayedManageableMailQueueContract.java
##########
@@ -70,6 +72,61 @@ default void delayedMessagesShouldBeCleared() throws Exception {
assertThat(getManageableMailQueue().getSize()).isEqualTo(0L);
}
+ @Test
+ default void delayedEmailsShouldBeDeleted() throws Exception {
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("abc")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+ // The queue being FIFO a second email can serve as a wait condition
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("def")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ getManageableMailQueue().remove(ManageableMailQueue.Type.Name, "abc");
+
+ ArrayList<String> names = new ArrayList<>();
+ Flux.from(getManageableMailQueue().deQueue())
+ .subscribeOn(Schedulers.elastic())
+ .subscribe(item -> names.add(item.getMail().getName()));
+
+ Awaitility.await().untilAsserted(() -> assertThat(names).contains("def"));
+ assertThat(names).containsExactly("def");
+ }
+
+ @Test
+ default void delayedEmailsShouldBeDeletedWhenMixedWithOtherEmails() throws Exception {
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("abc")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ getManageableMailQueue().remove(ManageableMailQueue.Type.Name, "abc");
+
+ // The newer email
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("def")
+ .build());
+ // The queue being FIFO a third email can serve as a wait condition
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("ghi")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ ArrayList<String> names = new ArrayList<>();
+ Flux.from(getManageableMailQueue().deQueue())
+ .subscribeOn(Schedulers.elastic())
+ .subscribe(item -> names.add(item.getMail().getName()));
+
+ Awaitility.await().untilAsserted(() -> assertThat(names).contains("ghi"));
Review comment:
```suggestion
Awaitility.await()
.untilAsserted(() -> assertThat(names).contains("ghi"));
```
--
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 edited a comment on pull request #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa edited a comment on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007882993
> I suggest not starting a third discussion on the same topic.
Fair. But failing tests have added value and shows fonctionnal expectations I have.
It also makes the whole discussion less abstract (and show I care enough to dedicate a bit of time to the topic).
I do not feel like this just is valueless conversation duplication.
BTW it proved we have two distinct problems while the design review only allowed to catch one!
> so we implemented the simplest that passed the contracts :)
I have limited faight in these contracts, delays is not an area covered by the previous RabbitMQ implementation and we did devote limited amount of time testing edge cases, interactions with other MailQueue features. It should not be taken as "written in marble".
> attach a monotonously increasing identifier such as a timestamp
:+1:
That's what I meant by "We would likely need the sequence to be backed by some applicative metadata - like a timestamp?". Of course it brings additional problems like accounting for imperfect clock synchronisation with a tolerated clock skew.
> query the scheduled topic offset on filter creation and attach that to the filter in addition to the out topic offset. filter scheduled messages as they are consumed and before they are requeued in the out topic. a filter would then expire when both the out topic and the scheduled topic's offset have been exceeded. (a simple way to implement this with the current setup is to duplicate the filter actor and the filter command queue and reuse the existing logic for the scheduled queue)
:+1:
--
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 #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007882993
> I suggest not starting a third discussion on the same topic.
Fair. But failing tests have added value and shows fonctionnal expectations I have.
It also makes the whole discussion less abstract (and show I care enough to dedicate a bit of time to the topic).
I do not feel like this just is valueless conversation duplication.
BTW it proved we have two distinct problems while the design review only allowed to catch one!
> so we implemented the simplest that passed the contracts :)
I have limited faight in these contracts, delays is not an area covered by the previous RabbitMQ implementation and we did devote limited amount of time testing edge cases, interactions with other MailQueue features. It should not be taken as "written in marble".
> attach a monotonously increasing identifier such as a timestamp
That's what I meant by "We would likely need the sequence to be backed by some applicative metadata - like a timestamp?". Of course it brings additional problems like accounting for imperfect clock synchronisation with a tolerated clock skew.
> query the scheduled topic offset on filter creation and attach that to the filter in addition to the out topic offset. filter scheduled messages as they are consumed and before they are requeued in the out topic. a filter would then expire when both the out topic and the scheduled topic's offset have been exceeded. (a simple way to implement this with the current setup is to duplicate the filter actor and the filter command queue and reuse the existing logic for the scheduled queue)
:+1:
--
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 change in pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #831:
URL: https://github.com/apache/james-project/pull/831#discussion_r782739779
##########
File path: server/queue/queue-api/src/test/java/org/apache/james/queue/api/DelayedManageableMailQueueContract.java
##########
@@ -70,6 +72,61 @@ default void delayedMessagesShouldBeCleared() throws Exception {
assertThat(getManageableMailQueue().getSize()).isEqualTo(0L);
}
+ @Test
+ default void delayedEmailsShouldBeDeleted() throws Exception {
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("abc")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+ // The queue being FIFO a second email can serve as a wait condition
+ getManageableMailQueue().enQueue(defaultMail()
+ .name("def")
+ .build(),
+ 5L,
+ TimeUnit.SECONDS);
+
+ getManageableMailQueue().remove(ManageableMailQueue.Type.Name, "abc");
+
+ ArrayList<String> names = new ArrayList<>();
+ Flux.from(getManageableMailQueue().deQueue())
+ .subscribeOn(Schedulers.elastic())
+ .subscribe(item -> names.add(item.getMail().getName()));
+
+ Awaitility.await().untilAsserted(() -> assertThat(names).contains("def"));
Review comment:
```suggestion
Awaitility.await()
.untilAsserted(() -> assertThat(names).contains("def"));
```
--
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 merged pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #831:
URL: https://github.com/apache/james-project/pull/831
--
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 #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007949523
> in other words, removal applies on enqueue order and not on dequeue order.
Yes.
Imagine for instance a given sender uses my SaaS instance to send SPAM. I spot the issue, dis-activate his account and purge the queue of his messages, both ready for sending and scheduled.
Also, similarily if a given recipient start bouncing, we wmight have a lot of messages in the queue, both without delays and with delays (retries). If the situation is hopeless, I might want to remove all emails to that very recipient, both scheduled and not scheduled.
> Assuming we implement the 2nd option I proposed : duplicate the filter logic for items consumed from the scheduled topic before they are injected in the out topic using the schedule topic offset at the time of remove as a reference for filtering, do you still see
:+1:
>> Second problem: filter removal based on sequence number
> as a fundamental problem that needs to be solved ? I can't think of anything but I could be missing something.
I do not understand your reply
If the reference to apply deletes is the time of enqueue, as we have delays, these would thus not be ordered at the time of dequeue. Thus we could not craft an heuristic relying on an ordered sequence to simplify the deletion problem (performance versus correctness).
I do not see other solutions than keeping the filters forever (as we do not know if a message for which filter applies is still in the queue)
- We could define an upper bound to delays, thus allowing keeing the same strategy
- A smarter solution could be to :
- Upon delete browse the queue to find matching messages
- Build a distributed filter using the enqueueId (unique idenfier for this email transit) + expected time of dequeue = delay + time of enqueue
- We could cleanup filters (that here would apply for individual emails) after a threshold after the expected time of dequeue is reached. Like 1 day after the time of the dequeue.
Or a double queuing system could be used:
- one topic for emails marked as deleted (builds the filter)
- one topic for emails effectively deleted (cleans the filter)
- Or we could document the limitation and keep it for later. Thus keeping the filters forever.
--
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] jeantil commented on pull request #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007568328
--
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 #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007547937
CC @jeantil @mbaechler
--
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 #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1011663643
@jeantil do you think I can merge 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.
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] jeantil commented on pull request #831: JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1011889129
yes
--
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] jeantil commented on pull request #831: WIP JAMES-3687 Demonstrate issues with deletes of delayed mails
Posted by GitBox <gi...@apache.org>.
jeantil commented on pull request #831:
URL: https://github.com/apache/james-project/pull/831#issuecomment-1007568328
I suggest not starting a third discussion on the same topic. as I [wrote in the first discussion](https://github.com/apache/james-project/pull/808#discussion_r780170243) the behavior for this case was not specified in the contracts so we implemented the simplest that passed the contracts :)
With that said, implementing the requested behavior is not so difficut. I propose two options depending on the degree of accuracy we require :
- attach a monotonously increasing identifier such as a timestamp (or an ObjectId for increased precision) to all enqueued messages and use that in the filter. This is sensitive to clock drift which could lead to some missed messages (not sure if it's an issue)
- query the scheduled topic offset on filter creation and attach that to the filter in addition to the out topic offset. filter scheduled messages as they are consumed and before they are requeued in the out topic. a filter would then expire when both the out topic and the scheduled topic's offset have been exceeded. (a simple way to implement this with the current setup is to duplicate the filter actor and the filter command queue and reuse the existing logic for the scheduled queue)
--
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