You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "Arooba-git (via GitHub)" <gi...@apache.org> on 2023/07/23 09:29:54 UTC
[GitHub] [james-project] Arooba-git opened a new pull request, #1653: Move blocking readPair call to boundedElastic thread
Arooba-git opened a new pull request, #1653:
URL: https://github.com/apache/james-project/pull/1653
Hi! 🙂
We notice 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:
<img width="1302" alt="james3-blocking" src="https://github.com/apache/james-project/assets/56495631/ef9febfc-1bd4-4da1-8aee-142a7a4d5d8e">
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**
<img width="1388" alt="james3-heap-before" src="https://github.com/apache/james-project/assets/56495631/e46c6c8a-9908-4bde-87f8-a0d94b5f9f68">
**After**
<img width="1384" alt="james3-heap-after" src="https://github.com/apache/james-project/assets/56495631/98a6d517-2f57-4f38-8ec4-5dba50cd0270">
--
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] Arooba-git commented on a diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "Arooba-git (via GitHub)" <gi...@apache.org>.
Arooba-git commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1271738313
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,14 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ Mono.fromRunnable(() -> {
+ readRepair(mailbox, counters);
Review Comment:
@chibenwa I'm sorry could you guide how to access probability property here? 🤔
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,14 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ Mono.fromRunnable(() -> {
+ readRepair(mailbox, counters);
Review Comment:
@chibenwa I'm sorry could you guide how to access probability property 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
Re: [PR] Move blocking readPair call to boundedElastic thread [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #1653:
URL: https://github.com/apache/james-project/pull/1653#issuecomment-2647890990
Please provide a pseudo random implementation for read repairs
--
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 diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1271624422
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,14 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ Mono.fromRunnable(() -> {
+ readRepair(mailbox, counters);
Review Comment:
Can we manage to subscribe here **only if read repairs are enabled** ie probability > 0 ?
--
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 diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1272990093
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,16 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {
Review Comment:
Maybe we could extract that line actually in a function to avoid duplication, that we could call from there and from your code change :)
--
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 #1653: Move blocking readPair call to boundedElastic thread
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #1653:
URL: https://github.com/apache/james-project/pull/1653#issuecomment-1649204817
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?
--
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] Arooba-git commented on a diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "Arooba-git (via GitHub)" <gi...@apache.org>.
Arooba-git commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1272406367
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,16 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {
Review Comment:
Oh, you mean we should update the name in the original configuration file too? 🤔
<img width="699" alt="Screen Shot 2023-07-25 at 12 13 15 AM" src="https://github.com/apache/james-project/assets/56495631/5861434b-41cb-4aa3-9d18-d1e610045ea9">
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,16 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {
Review Comment:
Oh, you mean we should update the name in the original configuration file too? 🤔
<img width="699" alt="Screen Shot 2023-07-25 at 12 13 15 AM" src="https://github.com/apache/james-project/assets/56495631/5861434b-41cb-4aa3-9d18-d1e610045ea9">
--
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] Move blocking readPair call to boundedElastic thread [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa closed pull request #1653: Move blocking readPair call to boundedElastic thread
URL: https://github.com/apache/james-project/pull/1653
--
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] Arooba-git commented on a diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "Arooba-git (via GitHub)" <gi...@apache.org>.
Arooba-git commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1271672713
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,14 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ Mono.fromRunnable(() -> {
+ readRepair(mailbox, counters);
Review Comment:
Sure, you mean something like:
```
.doOnNext(counters -> {
if (probability > 0) {
Mono.fromRunnable(() -> {
readRepair(mailbox, counters);
})
.subscribeOn(Schedulers.boundedElastic())
.subscribe();
}
});
```
--
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 diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1271721199
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,14 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ Mono.fromRunnable(() -> {
+ readRepair(mailbox, counters);
Review Comment:
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] chibenwa commented on a diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1271770670
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,14 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ Mono.fromRunnable(() -> {
+ readRepair(mailbox, counters);
Review Comment:
from the configuration
--
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 diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1272365385
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,16 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {
Review Comment:
I'm oretty sure it should be
```suggestion
if (this.cassandraConfiguration.getMailboxCountersReadRepair() > 0) {
```
--
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 diff in pull request #1653: Move blocking readPair call to boundedElastic thread
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #1653:
URL: https://github.com/apache/james-project/pull/1653#discussion_r1272989626
##########
mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageMapper.java:
##########
@@ -170,7 +170,16 @@ public Mono<MailboxCounters> getMailboxCountersReactive(Mailbox mailbox) {
}
return Mono.just(counters);
})
- .doOnNext(counters -> readRepair(mailbox, counters));
+ .doOnNext(counters -> {
+ if (this.cassandraConfiguration.getMailboxReadRepair() > 0) {
Review Comment:
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
--
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