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-3204 Push limit to Cassandra backend when reading messages #3428

Closed
wants to merge 8 commits into from

Conversation

chibenwa
Copy link
Member

@chibenwa chibenwa commented Jun 8, 2020

We noticed some BusyPool exceptions filling up the driver queue upon IMAP query (FETCH flags for all the messages in the mailbox).

The MessageManager do batch those reads (by default by 200 for metadata)., wich then call the MessageMapper with this limit.

Some unit tests performed at the Cassandra mailbox level proved the soft filtering did badly applies, and that we were performing uneeded extra reads for the full batch read from the database.

One good mitigation strategy is to push the limit to the Cassandra query, and ensures filtering happens before the extra reads are performed.

https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Flux.html#take-long- documents that it just don't propagate downstream request once a given amount is reached however it do not ensure any form of backpressure. We might want to further audit our code, looking for similar take mis-usages.

@chibenwa chibenwa added the perf Contributes some performance enhencements label Jun 8, 2020
@chibenwa chibenwa added this to the Polish 2020-06 milestone Jun 8, 2020
@chibenwa chibenwa self-assigned this Jun 8, 2020
@chibenwa
Copy link
Member Author

chibenwa commented Jun 8, 2020

The impact of other usages of Flux::take are likely limited.

@chibenwa
Copy link
Member Author

chibenwa commented Jun 8, 2020

MemoryTerminationSubscriberTest.dynamicListeningEventsShouldGetOnlyNewEvents

test this please

@chibenwa
Copy link
Member Author

chibenwa commented Jun 9, 2020

[ERROR] error reading /root/.m2/repository/com/github/fge/throwing-lambdas/0.5.0/throwing-lambdas-0.5.0.jar; zip file is empty

Let's try that again

test this please

Copy link

@mbaechler mbaechler left a comment

Choose a reason for hiding this comment

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

This look correct but the StatementRecorder. Could you fix it?

@chibenwa
Copy link
Member Author

This look correct but the StatementRecorder. Could you fix it?

Of course.

@mbaechler mbaechler added the requires-load-testing Running load testing to back these changes is required for adoption label Jun 10, 2020
@mbaechler
Copy link

I set the requires-load-testing flag

Copy link

@blackheaven blackheaven left a comment

Choose a reason for hiding this comment

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

need rebase

@chibenwa
Copy link
Member Author

Force pushed to solve a rebase conflict

@chibenwa
Copy link
Member Author

chibenwa commented Jun 11, 2020

mbaechler added the requires-load-testing label 21 hours ago

Let me argue that it don't as I prove to reduce the query count by a x25 factor for some basic JMAP operations.

I will be waiting for feedback about this decision but I would like that change to be part of the next James release for OpenPaaS as it solves some of our customer concerns.

@chibenwa chibenwa added the waiting_merge We are about to merge this! label Jun 11, 2020
@rouazana
Copy link

merged

@rouazana rouazana closed this Jun 11, 2020
@mbaechler
Copy link

Let me argue that it don't as I prove to reduce the query count by a x25 factor for some basic JMAP operations.

Well, that would not be the first time in my work life that things are not going as expected.

Checking a property we expect is usual practice. Why should we avoid that?

@chibenwa
Copy link
Member Author

Checking a property we expect is usual practice. Why should we avoid that?

I tried but couldn't run IMAP scenari on the charge CI...

Anyway, there still is a task about evaluating the performance impact of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf Contributes some performance enhencements requires-load-testing Running load testing to back these changes is required for adoption waiting_merge We are about to merge this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants