JAMES-2813 Add non-polling and timeout on TaskManager.await #2653

blackheaven · 2019-08-30T08:12:03Z

No description provided.

Arsnael · 2019-08-30T08:24:37Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

-        @ApiResponse(code = HttpStatus.NOT_FOUND_404, message = "The taskId is not found")
+        @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = "The taskId is invalid or invalid timeout"),
+        @ApiResponse(code = HttpStatus.NOT_FOUND_404, message = "The taskId is not found"),
+        @ApiResponse(code = HttpStatus.REQUEST_TIMEOUT_408, message = "The timeout have been reached")


Arsnael · 2019-08-30T08:24:54Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

+                    ErrorResponder.builder()
+                        .statusCode(HttpStatus.REQUEST_TIMEOUT_408)
+                        .type(ErrorResponder.ErrorType.SERVER_ERROR)
+                        .message("The timeout have been reached")


Arsnael · 2019-08-30T08:25:50Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+            .queryParam("timeout", "5")
+        .when()
+            .get("/" + taskId.getValue() + "/await")
+            .then()


Arsnael · 2019-08-30T08:26:06Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+
+        given()
+            .queryParam("timeout", "-5")
+            .when()


That went back somehow in one of your fixup?

a github bug I think

Arsnael · 2019-08-30T08:26:09Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+            .queryParam("timeout", "-5")
+            .when()
+            .get("/" + taskId.getValue() + "/await")
+            .then()


That went back somehow in one of your fixup?

Arsnael · 2019-08-30T08:26:19Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+
+        given()
+            .queryParam("timeout", "2")
+            .when()


Arsnael · 2019-08-30T08:26:22Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+            .queryParam("timeout", "2")
+            .when()
+            .get("/" + taskId.getValue() + "/await")
+            .then()


Arsnael · 2019-08-30T08:26:28Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+    void getAwaitWithANonExistingTaskShouldReturnNotFound() {
+        when()
+            .get("/" + TaskId.generateTaskId().asString() + "/await")
+            .then()


Arsnael · 2019-08-30T08:27:55Z

server/task/src/test/java/org/apache/james/task/TaskManagerContract.java

+    }
+
+    @Test
+    default void awaitANonExistingTaskShouldReturnAUnknownAwaitedTaskExecutionDetailsAndThrow() {


s/AUnknown/AnUnknown

Arsnael · 2019-08-30T08:28:55Z

[235366ef6e4e04ad3005692b8272b7ab43b89696] [INFO] There is 1 error reported by Checkstyle 8.18 with checkstyle.xml ruleset.
[235366ef6e4e04ad3005692b8272b7ab43b89696] [ERROR] src/main/java/org/apache/james/task/eventsourcing/distributed/RabbitMQWorkQueue.java:[29,8] (imports) UnusedImports: Unused import - org.apache.commons.lang3.NotImplementedException.

rouazana · 2019-08-30T09:21:15Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

+                .orElse(Long.MAX_VALUE / 1000);
+
+            Preconditions.checkState(value > 0);
+            return Duration.ofSeconds(value);


why do you divide by 1000 and the take a Duration.ofSeconds? You could directly take a Duration.ofMillis, no?

ok I got the explanation, but the readability is not very clear. If you name Long.MAX_VALUE as a constant, it would be better.

The unit in the API is seconds and the maximal value of Duration is Duration.ofMillis(Long.MAX_VALUE), I'm forced to divide to avoid overflows.

Who will need to wait an HTTP request for 106751991167300 days? What if you assert a max value of 1 day for example?

BTW, there's no test to check this "feature"

TasksRoutesTests.getAwaitWithInvalidTimeoutShouldReturnAnError()

I prefer let the user the choice

It doesn't check that you can put 106751991167300 days

rouazana · 2019-08-30T09:22:31Z

src/site/markdown/server/manage-webadmin.md

@@ -3269,16 +3269,17 @@ One can await the end of a task, then receive it's final execution report.
 That feature is especially usefull for testing purpose but still can serve real-life scenari.

 ```
-curl -XGET http://ip:port/tasks/3294a976-ce63-491e-bd52-1b6f465ed7a2/await
+curl -XGET http://ip:port/tasks/3294a976-ce63-491e-bd52-1b6f465ed7a2/await?timeout=numberOfSeconds


as you divide by 1000 it's millis, no?
BTW what about allowing some timeunit here? I guess we already have all the needed available code for that.

I've found nothing regarding that.

Anyway, I think it clearly not worth the time and effort maintaining such code and a decent suite of tests.

Is that new timeout parameter now compulsory?

org.apache.james.util.DurationParser?

Is that new timeout parameter now compulsory?

no

org.apache.james.util.DurationParser?

spaces are mandatory, it look likes a bad idea

that's not how I read it. But maybe some missing tests

rouazana · 2019-08-30T11:49:15Z

[a00bcb5801ef53778c791f98b374a1c8c0759985] [ERROR] src/main/java/org/apache/james/DistributedTaskManagerModule.java:[26,8] (imports) UnusedImports: Unused import - org.apache.james.task.eventsourcing.MemoryTerminationSubscriber.

mbaechler · 2019-08-30T12:27:15Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

-        @ApiResponse(code = HttpStatus.NOT_FOUND_404, message = "The taskId is not found")
+        @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = "The taskId is invalid or invalid timeout"),
+        @ApiResponse(code = HttpStatus.NOT_FOUND_404, message = "The taskId is not found"),
+        @ApiResponse(code = HttpStatus.REQUEST_TIMEOUT_408, message = "The timeout have been reached")


do you think the message add anything?

sure, because it specifies the cause of the request timeout

do you know other timeout cause than "The timeout has been reached"?

The server has reached it's life timeout
The user should have reach her end of life

More seriously I'm not sure it means exactly what it should (see https://tools.ietf.org/html/rfc7231#section-6.5.7 ), but I'm fine with it anyway.

can the result when reached timeout be represented in by 200 response code and a Json body telling that the task is in processing? and the message will be more clear than just a status code

A 200 status code for an exceeding timeout, I disagree, we submit a request, which is the specification of a process, we can't fulfill the requirements, we should not say that it's ok.

mbaechler · 2019-08-30T12:34:41Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

+                .orElse(Long.MAX_VALUE / 1000);
+
+            Preconditions.checkState(value > 0);
+            return Duration.ofSeconds(value);


Who will need to wait an HTTP request for 106751991167300 days? What if you assert a max value of 1 day for example?

mbaechler · 2019-08-30T12:35:34Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

+                .orElse(Long.MAX_VALUE / 1000);
+
+            Preconditions.checkState(value > 0);
+            return Duration.ofSeconds(value);


BTW, there's no test to check this "feature"

mbaechler · 2019-08-30T12:37:59Z

server/task/src/main/scala/org/apache/james/task/eventsourcing/EventSourcingTaskManager.scala

+        new TerminatedAwaitedTaskExecutionDetails(getExecutionDetails(id))
+      }
+    } catch {
+      case _: IllegalStateException => new TimeoutAwaitedTaskExecutionDetails()


do you need this _: Foo syntax ?

rouazana · 2019-08-30T13:10:07Z

[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR] Tests run: 6, Failures: 0, Errors: 4, Skipped: 0, Time elapsed: 1.262 s <<< FAILURE! - in org.apache.james.task.eventsourcing.MemoryTerminationSubscriberTest
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR] handlingCompletedShouldBeListed  Time elapsed: 0.225 s  <<< ERROR!
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] reactor.core.Exceptions$ReactiveException: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] Caused by: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR] handlingCancelledShouldBeListed  Time elapsed: 0.206 s  <<< ERROR!
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] reactor.core.Exceptions$ReactiveException: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] Caused by: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR] handlingMultipleEventsShouldBeListed  Time elapsed: 0.206 s  <<< ERROR!
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] reactor.core.Exceptions$ReactiveException: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] Caused by: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR] handlingFailedShouldBeListed  Time elapsed: 0.205 s  <<< ERROR!
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] reactor.core.Exceptions$ReactiveException: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] Caused by: java.lang.InterruptedException
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] Running org.apache.james.task.eventsourcing.EventSourcingTaskManagerTest
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] Tests run: 30, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.243 s - in org.apache.james.task.eventsourcing.EventSourcingTaskManagerTest
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] Running org.apache.james.task.SerialTaskManagerWorkerTest
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.602 s - in org.apache.james.task.SerialTaskManagerWorkerTest
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] Results:
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [INFO] 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR] Errors: 
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR]   MemoryTerminationSubscriberTest.handlingCancelledShouldBeListed » Reactive jav...
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR]   MemoryTerminationSubscriberTest.handlingCompletedShouldBeListed » Reactive jav...
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR]   MemoryTerminationSubscriberTest.handlingFailedShouldBeListed » Reactive java.l...
[7e4f80abe2729d080169d9ba3ae7cc0fdfcb324d] [ERROR]   MemoryTerminationSubscriberTest.handlingMultipleEventsShouldBeListed » Reactive

trantienduchn · 2019-09-03T03:49:27Z

[e8e5929d9557e4d54330c634befa58229327cc5d] 2 errors>
[e8e5929d9557e4d54330c634befa58229327cc5d] to be an instance of:
[e8e5929d9557e4d54330c634befa58229327cc5d]   <org.apache.james.StartUpChecksPerformer.StartUpChecksException>
[e8e5929d9557e4d54330c634befa58229327cc5d] but was:
[e8e5929d9557e4d54330c634befa58229327cc5d]   <"com.google.inject.CreationException: Unable to create injector, see the following errors:
[e8e5929d9557e4d54330c634befa58229327cc5d] 
[e8e5929d9557e4d54330c634befa58229327cc5d] 1) No implementation for org.apache.james.task.eventsourcing.TerminationSubscriber was bound.
[e8e5929d9557e4d54330c634befa58229327cc5d]   Did you mean?
[e8e5929d9557e4d54330c634befa58229327cc5d]     org.apache.james.task.eventsourcing.TerminationSubscriber bound  at org.apache.james.modules.server.TaskManagerModule.configure(TaskManagerModule.java:37) (via modules: com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$OverrideModule -> com.google.inject.util.Modules$CombinedModule -> com.google.inject.util.Modules$CombinedModule -> org.apache.james.modules.CommonServicesModule -> org.apache.james.modules.server.TaskManagerModule)

trantienduchn · 2019-09-03T04:04:34Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

-        @ApiResponse(code = HttpStatus.NOT_FOUND_404, message = "The taskId is not found")
+        @ApiResponse(code = HttpStatus.BAD_REQUEST_400, message = "The taskId is invalid or invalid timeout"),
+        @ApiResponse(code = HttpStatus.NOT_FOUND_404, message = "The taskId is not found"),
+        @ApiResponse(code = HttpStatus.REQUEST_TIMEOUT_408, message = "The timeout have been reached")


can the result when reached timeout be represented in by 200 response code and a Json body telling that the task is in processing? and the message will be more clear than just a status code

trantienduchn · 2019-09-03T04:08:23Z

server/task/src/main/scala/org/apache/james/task/eventsourcing/EventSourcingTaskManager.scala

-    } else {
-      Flux.from(terminationSubscriber.listenEvents)
-          .filter{
+  override final def await(id: TaskId, timeout: Duration): AwaitedTaskExecutionDetails = {


don't you think Optional<Duration> could be better?

I think the submit() need to support timeout too

No, because you would not not if you had an Option.empty due to an TaskNotFound or a TimeOut.

You want it to fail if you can not submit a Task.

blackheaven · 2019-09-03T14:56:02Z

[ERROR]   CassandraMailRepositoryTest.storingAndRemovingMessagesConcurrentlyShouldLeadToConsistentResult » Execution

test this please

chibenwa · 2019-09-04T01:54:42Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+            .get("/" + taskId.getValue() + "/await")
+        .then()
+            .statusCode(HttpStatus.BAD_REQUEST_400)
+            .body("message", is("Invalid timeout"));


Why? (details?)

which details?

Why is the timeout invalid? Can you add it in the returned exception detailed field?

you pass one parameter, it is invalid, what would be the gain?

chibenwa · 2019-09-04T01:55:21Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+        });
+
+        given()
+            .queryParam("timeout", "2")


Can't we reduce it to 1s? 100ms? (faster tests?)

You won't gained much

1 to 2 second a test?

on 3 tests, we will survive

chibenwa · 2019-09-04T01:55:47Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

+    @Test
+    void getAwaitWithANonExistingTaskShouldReturnNotFound() {
+        when()
+            .get("/" + TaskId.generateTaskId().asString() + "/await")


Extract that variable to unknownTaskId?

cosmetic, I'll fix it if I have other modifications

not cosmetic IMO

chibenwa · 2019-09-04T01:56:54Z

...main/java/org/apache/james/task/eventsourcing/distributed/RabbitMQTerminationSubscriber.java

+        try {
+            byte[] payload = serializer.serialize(event).getBytes(StandardCharsets.UTF_8);
+            AMQP.BasicProperties basicProperties = new AMQP.BasicProperties.Builder().build();
+            OutboundMessage message = new OutboundMessage(EXCHANGE_NAME, ROUTING_KEY, basicProperties, payload);


I see that Event => OutboundMessage serialization a bit everywhere. Wouldn't it make sens to factorize it?

Not really, we have multiple ways to serialize them

chibenwa · 2019-09-04T01:57:32Z

...main/java/org/apache/james/task/eventsourcing/distributed/RabbitMQTerminationSubscriber.java

+            .share();
+    }
+
+    private Mono<Event> toEvent(Delivery delivery) {


Idem for that Delivery => Event.

chibenwa · 2019-09-04T01:59:03Z

server/task/src/main/java/org/apache/james/task/TaskManager.java

+        }
+
+        public TaskExecutionDetails unwrap() {
+            throw new RuntimeException("await has failed due to timeout");


Can't we use a more specific exception?

no, it's a call issue, it's not supposed to happen in production

chibenwa · 2019-09-04T02:00:32Z

server/task/src/main/java/org/apache/james/task/TaskManager.java

+        }
+
+        public TaskExecutionDetails unwrap() {
+            throw new RuntimeException("await has failed due to unknown task");


Can we use a more specific exception?

chibenwa · 2019-09-04T02:02:14Z

server/task/src/main/java/org/apache/james/task/TaskManager.java

@@ -58,6 +60,62 @@ public boolean isFinished() {
        }
    }

+    interface AwaitedTaskExecutionDetails {


Isn't this a disguised sketchy staged builder?

A staged builder of one element? it will only make it heavier for no benefit

Well I have the impression that you return several time invalid variations of the same entity, for which calling unwrap fails with a not specific exception.

You require IMO too much knowledge from the caller (if I use it, I will break it) and I fail to understand the way this "thing" - that do not follow patterns that I know - work.

Can we try to review and rediscuss code organization here?

No, and I won't fix it.

You'll loose in expressiveness (have a look in at tests)

You'll add burden to code which is already complex adding no value

You have to test the code you produce (including using this one), so you'll be corrected quickly

It's perfectly acceptable to have code not behaving the way we are use to, that does not make it a bad code (rephrasing: it's not because we know things that work, that there is nothing we do not know that work too)

chibenwa · 2019-09-04T07:32:15Z

server/task/src/test/java/org/apache/james/task/TaskManagerContract.java

+
+        assertThatCode(() -> result
+                .onTimeout(CustomTimeoutException::new)
+                .onUnknown(CustomUnknownException::new)


Why is it the responsability of the caller to instanciate the exceptions?

Why can't we have well defined exceptions thrown directly by the await method?

Sorry I don't get the benefits behind all of this complexity...

Why is it the responsability of the caller to instanciate the exceptions?

I do now instantiate it here, I pass the method reference

Why can't we have well defined exceptions thrown directly by the await method?

Because reaching a timeout is not a real exception, consequently, it should not be manage like that

Sorry I don't get the benefits behind all of this complexity...

ADT are not complex, RunTimeExceptions are

I do now instantiate it here, I pass the method reference

Thanks I understand

Because reaching a timeout is not a real exception, consequently, it should not be manage like that

Thanks that's a beginning of answer

ADT are not complex, RunTimeExceptions are

Who said that it should be runtime?

Who said that it should be runtime?

Well, if it's not passed as a value, the only other non too-dirty way I see is an Exception

That would be more straightforward, no?

chibenwa · 2019-09-05T10:30:06Z

...s/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/routes/TasksRoutesTest.java

@@ -223,7 +225,7 @@ void getAwaitWithInvalidTimeoutShouldReturnAnError() {
            .get("/" + taskId.getValue() + "/await")
        .then()
            .statusCode(HttpStatus.BAD_REQUEST_400)
-            .body("message", is("Invalid timeout"));
+            .body("message", allOf(containsString("Invalid timeout"), containsString("Timeout should not be positive")));


Wait... Shouldn't it be Timeout should be positive

chibenwa

Once the typo in the webadmin error message is fixed

blackheaven · 2019-09-05T15:38:13Z

[272c72e69ea0fc5655cb47d64c427c5b5b8e130d] Expected: '\* SEARCH 1 2 3 4'
[272c72e69ea0fc5655cb47d64c427c5b5b8e130d] Actual   : '* SEARCH'

test this please

chibenwa · 2019-09-05T16:30:09Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

@@ -196,7 +196,7 @@ private Duration getTimeout(Request req) {
                .filter(parameter -> !parameter.isEmpty());

            requestTimeout.ifPresent(timeout ->
-                Preconditions.checkState(!timeout.replaceAll(" ", "").startsWith("-"), "Timeout should not be positive"));
+                Preconditions.checkState(!timeout.replaceAll(" ", "").startsWith("-"), "Timeout should be positive"));


Checking the first character is a strange way of ensuring a number is negative...

(Sorry, I missed it so far)

It's the only way I've founded to check it, since an Integer.parseXXX would not recognize the unit and duration parsing won't recognize the minus sign, throwing a generic error

Let me have a look...

See 2f10137

chibenwa · 2019-09-05T16:34:10Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

+                .map(rawString -> DurationParser.parse(rawString, ChronoUnit.SECONDS))
+                .orElse(MAXIMUM_AWAIT_TIMEOUT);
+
+            Preconditions.checkState(timeout.compareTo(MAXIMUM_AWAIT_TIMEOUT) <= 0, "Timeout should not exceed one year");


A precondition at the end of a method look weird. Moving it to a ensureXxx method would solve this.

I did not find an example, can you give me one please?

See See 2f10137

chibenwa · 2019-09-05T16:35:50Z

Overwise 👍

Arsnael · 2019-09-06T04:01:48Z

tests were 🍏

[272c72e69ea0fc5655cb47d64c427c5b5b8e130d] ADD failed: stat /var/lib/docker/tmp/docker-builder652555734/destination/james-server-app-3.4.0-SNAPSHOT-app.zip: no such file or directory

test this please

blackheaven · 2019-09-06T14:09:13Z

test this please

chibenwa · 2019-09-09T02:00:50Z

test this please

blackheaven · 2019-09-09T12:18:01Z

test this please

mbaechler · 2019-09-09T14:15:55Z

...ocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/routes/TasksRoutes.java

+    }
+
+    private void assertDoesNotExceedMaximumTimeout(Duration timeout) {
+        Preconditions.checkState(timeout.compareTo(MAXIMUM_AWAIT_TIMEOUT) <= 0, "Timeout should not exceed one year");


s/one year/365 days/

mbaechler · 2019-09-09T14:17:53Z

server/task/src/main/java/org/apache/james/task/TaskManager.java

@@ -58,6 +59,9 @@ public boolean isFinished() {
        }
    }

+    class ReachedTimeoutException extends Exception {


why the hell a checked Exception?

to force handling it

usually we use a RuntimeException for these cases, and explicitly state that the method is throwing it with a throws MyRuntimeException. What's the point here to force all the callers to handle it?

We forgot to handle it at the right places, it would be a shame to allow this kind of bug to happen again. It's a design fix.

Arsnael · 2019-09-10T01:44:32Z

[36d74dde41ae70ebd83f98858c6307582fe1d56a] [ERROR] Failures: 
[36d74dde41ae70ebd83f98858c6307582fe1d56a] [ERROR]   DelayTest.stringConstructorShouldThrowOnNegativeNumbers 
[36d74dde41ae70ebd83f98858c6307582fe1d56a] Expected: an instance of java.lang.NumberFormatException
[36d74dde41ae70ebd83f98858c6307582fe1d56a]      but: <java.lang.IllegalArgumentException: Duration amount should be positive> is a java.lang.IllegalArgumentException

blackheaven · 2019-09-10T11:20:31Z

ADD failed: stat /var/lib/docker/tmp/docker-builder143880794/destination/james-server-app-3.4.0-SNAPSHOT-app.zip: no such file or directory

can be considered 🍏

This enables a nicer error message upon negative numbers, avoiding hacks on the caller side.

Arsnael · 2019-09-13T03:19:16Z

CassandraMailRepositoryTest.storingAndRemovingMessagesConcurrentlyShouldLeadToConsistentResult » Execution

test this please

remk · 2019-09-18T09:28:02Z

test this please

rouazana · 2019-09-18T14:22:20Z

server/task/src/test/java/org/apache/james/task/TaskManagerContract.java

    }

    @Test
-    default void awaitShouldAwaitWaitingTask() {
+    default void awaitShouldAwaitWaitingTask() throws TaskManager.ReachedTimeoutException, InterruptedException {


throws Exception is sufficient for a test

rouazana · 2019-09-18T14:24:13Z

server/task/src/main/java/org/apache/james/task/TaskManager.java

@@ -58,6 +59,9 @@ public boolean isFinished() {
        }
    }

+    class ReachedTimeoutException extends Exception {


usually we use a RuntimeException for these cases, and explicitly state that the method is throwing it with a throws MyRuntimeException. What's the point here to force all the callers to handle it?

chibenwa · 2019-09-24T09:27:18Z

Merged

Arsnael reviewed Aug 30, 2019

View reviewed changes

blackheaven force-pushed the JAMES-2813-await branch from 235366e to a00bcb5 Compare August 30, 2019 09:10

rouazana reviewed Aug 30, 2019

View reviewed changes

blackheaven force-pushed the JAMES-2813-await branch from a00bcb5 to 7e4f80a Compare August 30, 2019 12:07

mbaechler reviewed Aug 30, 2019

View reviewed changes

blackheaven force-pushed the JAMES-2813-await branch from 7e4f80a to 9e0b888 Compare September 2, 2019 13:50

remk force-pushed the JAMES-2813-await branch from 9e0b888 to e8e5929 Compare September 2, 2019 15:01

trantienduchn reviewed Sep 3, 2019

View reviewed changes

blackheaven force-pushed the JAMES-2813-await branch 2 times, most recently from d061946 to 49f6576 Compare September 3, 2019 09:06

rouazana approved these changes Sep 3, 2019

View reviewed changes

chibenwa reviewed Sep 4, 2019

View reviewed changes

blackheaven force-pushed the JAMES-2813-await branch from 9f86816 to ae5ca97 Compare September 5, 2019 10:27

chibenwa reviewed Sep 5, 2019

View reviewed changes

chibenwa approved these changes Sep 5, 2019

View reviewed changes

rouazana approved these changes Sep 5, 2019

View reviewed changes

chibenwa reviewed Sep 5, 2019

View reviewed changes

Arsnael approved these changes Sep 6, 2019

View reviewed changes

Arsnael approved these changes Sep 9, 2019

View reviewed changes

chibenwa approved these changes Sep 9, 2019

View reviewed changes

mbaechler reviewed Sep 9, 2019

View reviewed changes

chibenwa approved these changes Sep 10, 2019

View reviewed changes

Arsnael approved these changes Sep 10, 2019

View reviewed changes

blackheaven and others added 13 commits September 12, 2019 15:53

JAMES-2813 Add cancel support in DistributedTaskManager

09692b3

JAMES-2813 Add non-polling await in DistributedTaskManager

e1fb2e4

JAMES-2813 Add timeout in TaskManager.await

b3faa03

fixup! JAMES-2813 Add timeout in TaskManager.await

9f03feb

fixup! JAMES-2813 Add timeout in TaskManager.await

6538812

fixup! JAMES-2813 Add timeout in TaskManager.await

f033b29

fixup! JAMES-2813 Add timeout in TaskManager.await

f9c033e

JAMES-2813 Enhance Duration parser negative number parsing

3b543a3

This enables a nicer error message upon negative numbers, avoiding hacks on the caller side.

fixup! JAMES-2813 Add timeout in TaskManager.await

1ecb9a8

fixup! JAMES-2813 Enhance Duration parser negative number parsing

857b0b3

fixup! JAMES-2813 Enhance Duration parser negative number parsing

825b6ca

fixup! JAMES-2813 Enhance Duration parser negative number parsing

9552d43

fixup! JAMES-2813 Add timeout in TaskManager.await

0585eac

Arsnael force-pushed the JAMES-2813-await branch from f2aaa59 to 0585eac Compare September 12, 2019 08:53

remk approved these changes Sep 18, 2019

View reviewed changes

rouazana reviewed Sep 18, 2019

View reviewed changes

rouazana approved these changes Sep 19, 2019

View reviewed changes

chibenwa added the waiting_merge We are about to merge this! label Sep 24, 2019

chibenwa closed this Sep 24, 2019

JAMES-2813 Add non-polling and timeout on TaskManager.await #2653

JAMES-2813 Add non-polling and timeout on TaskManager.await #2653

Conversation

blackheaven commented Aug 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arsnael commented Aug 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouazana commented Aug 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaechler Sep 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouazana commented Aug 30, 2019

trantienduchn commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackheaven commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaechler Sep 2, 2019 •

edited