Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JAMES-3105 Corrective task for fixing mailbox counters #3185

Closed
wants to merge 16 commits into from

Conversation

chibenwa
Copy link
Member

@chibenwa chibenwa commented Mar 7, 2020

Cassandra maintains a per mailbox projection for message count and unseen message count.

As with any projection, it can go out of sync, leading to inconsistent results being returned to the client, which is not acceptable.

We need to expose a corrective task, resetting mailbox counters to their correct value.

The proposed endpoint is:

curl -XPST http://ip:port/mailbox?task=RecomputeMailboxCounters

This endpoints will:

  • List existing mailboxes
  • List their messages
  • Check them against their source of truth
  • Compute mailbox counter values
  • And reset the value of the counter if needed

Of course, this endpoint won't be friendly in the face of concurrent operations, which would be ignored during the recomputation of the counter of the given mailbox. However the source of truth is unaffected hence, upon rerunning the task, the result will be eventually correct.

To be noted that we rely on the "listing messages by mailbox" projection (that we recheck). Missing entries in there will be ignored until the given projection is healed (currently unsupported). We furthermore can piggy back a partial check of the message denormalization upon counter recomputation (partial because we cannot detect missing entries in the "list messages in mailbox" denormalization table)

@chibenwa chibenwa added the prod label Mar 7, 2020
@chibenwa chibenwa added this to the Sprint 15 - Robusta Beans milestone Mar 7, 2020
@chibenwa chibenwa self-assigned this Mar 7, 2020
Resetting counters is intrinsecly flowed in the face of concurrent operations.
Algorithm:

 - List existing mailboxes
 - List their messages
 - Check them against their source of truth
 - Compute mailbox counter values
 - And reset the value of the counter if needed

Limitations:

This won't be friendly in the face of concurrent operations, which
would be ignored during the recomputation of the counter of the
given mailbox. However the source of truth is unaffected hence,
upon rerunning the algorithm, the result will be eventually correct.

We rely on the "listing messages by mailbox" projection (that we
recheck). Missing entries in there will be ignored until the given
projection is healed (currently unsupported).
A simple wrapper around the already existing service...
Proposed endpoint:

```
curl -XPST http://ip:port/mailboxes?task=RecomputeMailboxCounters
```
…ers recomputation

We can piggy back a partial check of the message denormalization upon
counter recomputation (partial because we cannot detect missing
entries in the "list messages in mailbox" denormalization table)
Copy link

@trantienduchn trantienduchn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read it

Copy link
Member Author

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 with the fixups

@Arsnael
Copy link
Member

Arsnael commented Mar 9, 2020

[b0eab27c6af4153775212cbf14450033ff33874f] [ERROR] /james-project/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/task/RecomputeMailboxCountersService.java:[180,46] cannot find symbol
[b0eab27c6af4153775212cbf14450033ff33874f]   symbol:   method resetCounter(org.apache.james.mailbox.model.MailboxCounters)
[b0eab27c6af4153775212cbf14450033ff33874f]   location: variable counterDAO of type org.apache.james.mailbox.cassandra.mail.CassandraMailboxCounterDAO

will fix it

@chibenwa chibenwa added waiting_merge We are about to merge this! and removed cross-review needed labels Mar 9, 2020
@blackheaven
Copy link

@chibenwa I know that you are optimistic, but please, wait for the CI to be green before labeling "waiting_merge"

@chibenwa
Copy link
Member Author

chibenwa commented Mar 9, 2020

@chibenwa I know that you are optimistic, but please, wait for the CI to be green before labeling "waiting_merge"

I merge when it's green, that's the meaning

@chibenwa
Copy link
Member Author

chibenwa commented Mar 9, 2020

Merged

@chibenwa chibenwa closed this Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prod waiting_merge We are about to merge this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants