quantranhong1999 commented on code in PR #1651:
URL: https://github.com/apache/james-project/pull/1651#discussion_r1296617942
##########
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSubmissionSetMethodFutureReleaseContract.scala:
##########
@@ -384,8 +384,9 @@ trait EmailSubmissionSetMethodFutureReleaseContract {
`with`()
.header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
.body(request)
- .post
+ .post.prettyPeek()
Review Comment:
debug
##########
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSubmissionSetMethodFutureReleaseContract.scala:
##########
@@ -321,7 +321,7 @@ trait EmailSubmissionSetMethodFutureReleaseContract {
.addHeader(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
.setBody(requestAndre)
.build, new ResponseSpecBuilder().build)
- .post
+ .post.prettyPeek()
Review Comment:
debug
##########
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSubmissionSetMethodFutureReleaseContract.scala:
##########
@@ -297,7 +297,7 @@ trait EmailSubmissionSetMethodFutureReleaseContract {
`with`()
.header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
.body(request)
- .post
+ .post.prettyPeek()
Review Comment:
debug spotted
##########
server/protocols/jmap-rfc-8621-integration-tests/distributed-jmap-rfc-8621-integration-tests/src/test/java/org/apache/james/jmap/rfc8621/distributed/DistributedEmailSubmissionSetMethodFutureReleaseTest.java:
##########
@@ -58,11 +57,13 @@ public class
DistributedEmailSubmissionSetMethodFutureReleaseTest implements Ema
.deduplication()
.noCryptoConfig())
.searchConfiguration(SearchConfiguration.openSearch())
+ .mailQueueChoice(MailQueueChoice.PULSAR)
.build())
.extension(new DockerOpenSearchExtension())
.extension(new CassandraExtension())
.extension(new RabbitMQExtension())
.extension(new AwsS3BlobStoreExtension())
+ .extension(new DockerPulsarExtension())
Review Comment:
We no longer need this in
`DistributedEmailSubmissionSetMethodFutureReleaseTest` IMO. We can not test the
Pulsar delay here anyway.
##########
server/protocols/jmap/src/main/java/org/apache/james/jmap/JMAPConfiguration.java:
##########
@@ -114,11 +115,16 @@ public Builder maximumSendSize(Optional<Long>
maximumSendSize) {
return this;
}
+ public Builder supportsDelaySends(Optional<Boolean>
supportsDelaySends) {
+ this.supportsDelaySends = supportsDelaySends;
+ return this;
+ }
+
public JMAPConfiguration build() {
Preconditions.checkState(enabled.isPresent(), "You should specify
if JMAP server should be started");
return new JMAPConfiguration(enabled.get(), port,
emailQueryViewEnabled.orElse(false),
userProvisioningEnabled.orElse(true),
- defaultVersion.orElse(Version.DRAFT), maximumSendSize);
+ defaultVersion.orElse(Version.DRAFT), maximumSendSize,
supportsDelaySends.isPresent());
Review Comment:
What if supportsDelaySends is present but its value = false ?
##########
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSubmissionSetMethodFutureReleaseContract.scala:
##########
@@ -406,7 +407,7 @@ trait EmailSubmissionSetMethodFutureReleaseContract {
.addHeader(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
.setBody(requestAndre)
.build, new ResponseSpecBuilder().build)
- .post
+ .post.prettyPeek()
Review Comment:
debug
##########
server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailSubmissionSetMethodFutureReleaseContract.scala:
##########
@@ -384,8 +384,9 @@ trait EmailSubmissionSetMethodFutureReleaseContract {
`with`()
.header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
.body(request)
- .post
+ .post.prettyPeek()
+ println("Now: " + LocalDateTime.now(updatableTickingClock))
Review Comment:
debug
##########
server/protocols/jmap/src/test/java/org/apache/james/jmap/JMAPConfigurationTest.java:
##########
@@ -31,7 +31,7 @@ class JMAPConfigurationTest {
public static final boolean ENABLED = true;
public static final boolean DISABLED = false;
-
+ public static final boolean NO_SUPPORT_DELAYED_SEND = false;
@Test
Review Comment:
empty line please
##########
server/apps/distributed-app/docker-compose.yml:
##########
@@ -37,47 +32,37 @@ services:
- discovery.type=single-node
- DISABLE_INSTALL_DEMO_CONFIG=true
- DISABLE_SECURITY_PLUGIN=true
- networks:
- james:
- aliases:
- - elasticsearch
cassandra:
- image: cassandra:4.1.3
+ image: cassandra:3.11.10
ports:
- "9042:9042"
- healthcheck:
- test: [ "CMD", "cqlsh", "-e", "describe keyspaces" ]
- interval: 3s
- timeout: 20s
- retries: 5
- networks:
- - james
tika:
- image: apache/tika:2.8.0.0
- networks:
- - james
+ image: apache/tika:1.28.2
rabbitmq:
- image: rabbitmq:3.12.1-management
+ image: rabbitmq:3.9.18-management
ports:
- "5672:5672"
- "15672:15672"
- networks:
- - james
+
+ pulsar:
+ image: apachepulsar/pulsar:2.10.1
+ container_name: pulsar
Review Comment:
Let's create another docker-compose specific for Pulsar MailQueue (e.g
`docker-compose-with-pulsar.yml`) and do not change the current
`docker-compose.yml`.
Rationale: not everyone uses Pulsar.
##########
server/queue/queue-pulsar/src/main/scala/org/apache/james/queue/pulsar/PulsarMailQueueFactory.scala:
##########
@@ -44,7 +45,7 @@ class PulsarMailQueueFactory @Inject()(pulsarConfiguration:
PulsarConfiguration,
mimeMessageStore: Store[MimeMessage, MimeMessagePartsId],
mailQueueItemDecoratorFactory: MailQueueItemDecoratorFactory,
metricFactory: MetricFactory,
- gaugeRegistry: GaugeRegistry
+ gaugeRegistry: GaugeRegistry,
) extends MailQueueFactory[PulsarMailQueue] {
Review Comment:
redundant `,` ?
All changes in `PulsarMailQueueFactory` class are not needed IMO
##########
server/apps/distributed-app/sample-configuration/jmap.properties:
##########
@@ -6,6 +6,8 @@ enabled=true
tls.keystoreURL=file://conf/keystore
tls.secret=james72laBalle
+delay.sends.enabled=true
+
Review Comment:
Please note that this only work with Pulsar and not RabbitMQ.
BTW did we document this `delay.sends.enabled` configuration property in
`jmap.adoc` and others?
##########
server/queue/queue-pulsar/pom.xml:
##########
@@ -77,7 +83,6 @@
<dependency>
<groupId>${james.groupId}</groupId>
<artifactId>james-server-testing</artifactId>
- <scope>test</scope>
Review Comment:
Please do not include test dependency into production code
##########
server/queue/queue-pulsar/src/main/scala/org/apache/james/queue/pulsar/PulsarMailQueue.scala:
##########
@@ -159,9 +159,9 @@ class PulsarMailQueue(
case (payload, duration, enqueued) =>
duration match {
case _: Duration.Infinite =>
- (ProducerMessage(payload) -> enqueued)
+ ProducerMessage(payload) -> enqueued
case duration: FiniteDuration =>
- val deliverAt = ZonedDateTime.now().plus(duration.toJava).toInstant
+ val deliverAt: Instant =
ZonedDateTime.now().plus(duration.toJava).toInstant
Review Comment:
All changes in `PulsarMailQueue` class are not needed IMO. You can revert
those changes.
##########
server/apps/distributed-app/src/main/java/org/apache/james/CassandraRabbitMQJamesServerMain.java:
##########
@@ -205,6 +207,20 @@ public static GuiceJamesServer
createServer(CassandraRabbitMQJamesConfiguration
.combineWith(chooseJmapModule(configuration));
}
+ private static Module chooseMailQueue(CassandraRabbitMQJamesConfiguration
configuration) {
+ switch (configuration.getMailQueueChoice()) {
+ case PULSAR:
+ return new PulsarQueueModule();
Review Comment:
IMO we should write an Integration test class with Pulsar mail queue that
implements some basic James contract test e.g `JamesServerContract` to make
sure James can run normally at very basic operations.
##########
server/apps/distributed-app/sample-configuration/queue.properties:
##########
@@ -0,0 +1 @@
+queue.choice=PULSAR
Review Comment:
Should be RabbitMQ by default IMO.
Maybe we should create another `queue.properties` with Pulsar choice and
volume it into the pulsar docker compose?
##########
server/apps/distributed-app/docker-compose.yml:
##########
@@ -4,23 +4,18 @@ services:
james:
depends_on:
- cassandra:
- condition: service_healthy
- opensearch:
- condition: service_started
- tika:
- condition: service_started
- rabbitmq:
- condition: service_started
- s3:
- condition: service_started
+ - opensearch
Review Comment:
Please always rebase on the latest origin master, not the outdated local
master.
##########
server/apps/distributed-app/sample-configuration/queue.properties:
##########
@@ -0,0 +1 @@
+queue.choice=PULSAR
Review Comment:
BTW did we document this `queue.properties` somewhere?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
