Skip to content

Move blocking readPair call to boundedElastic thread - #1653

Closed
Arooba-git wants to merge 1 commit into
apache:masterfrom
Arooba-git:fix-blocking-call-in-mailbox
Closed

Move blocking readPair call to boundedElastic thread#1653
Arooba-git wants to merge 1 commit into
apache:masterfrom
Arooba-git:fix-blocking-call-in-mailbox

Conversation

@Arooba-git

@Arooba-git Arooba-git commented Jul 23, 2023

Copy link
Copy Markdown
Contributor

Hi! 🙂

We noticed you did a great job in ensuring the reactive modules indeed stay reactive end to end. The mailbox module, however, was discovered to still have a blocking call in CassandraMessageMapper class as detected by BlockHound:
james3-blocking

This PR fixes this code. We also re-ran the tests and verified the performance (in terms of heap usage) before and after the fix:

Before
james3-heap-before

After

james3-heap-after

@quantranhong1999 quantranhong1999 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution <3

.doOnNext(counters -> readRepair(mailbox, counters));
.doOnNext(counters -> {
Mono.fromRunnable(() -> {
readRepair(mailbox, counters);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we manage to subscribe here only if read repairs are enabled ie probability > 0 ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, you mean something like:

.doOnNext(counters -> {
       if (probability > 0) {
              Mono.fromRunnable(() -> {
                   readRepair(mailbox, counters);
              })
             .subscribeOn(Schedulers.boundedElastic())
             .subscribe();
       }
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chibenwa I'm sorry could you guide how to access probability property here? 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

from the configuration

@Arooba-git
Arooba-git force-pushed the fix-blocking-call-in-mailbox branch from d2c426a to dec0b5d Compare July 24, 2023 06:41
})
.doOnNext(counters -> readRepair(mailbox, counters));
.doOnNext(counters -> {
if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm oretty sure it should be

Suggested change
if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {
if (this.cassandraConfiguration.getMailboxCountersReadRepair() > 0) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, you mean we should update the name in the original configuration file too? 🤔
Screen Shot 2023-07-25 at 12 13 15 AM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe what chibenwa is saying is:

  • getMailboxReadRepair() is used in CassandraMailboxMapper
  • for CassandraMessageMapper , we use getMailboxCountersReadRepairChanceMax() and/or getMailboxCountersReadRepairChanceOneHundred()

=> for example: https://github.com/apache/james-project/blob/master/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java#L197

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we could extract that line actually in a function to avoid duplication, that we could call from there and from your code change :)

@chibenwa

Copy link
Copy Markdown
Contributor

Also read repair could be a good place for pseudo random usage aka non blocking...

There is no need for cryptographioc grade randomness for such an application.

Maybe otherall this could be a better fix. WDYT?

@chibenwa

Copy link
Copy Markdown
Contributor

Please provide a pseudo random implementation for read repairs

@chibenwa chibenwa closed this Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants