Styx Eventual class for asynchronous events. #309

mikkokar · 2018-10-11T12:30:28Z

Replaces StyxObservable with Eventual class, that asynchronously publishes a single event only.

A main difference between a Future and an Eventual supports deferred actions like Reactor Mono (in fact it is just a wrapper around Mono). That is, where as CompletableFuture action starts running straight away, an Eventual needs an explicit subscription to start the action.

The codebase still needs some tidying up, but you should get an impression how the final API will look like.

2. Transport: Add FSM to drive the connection pool returns and cancellations. 3. Add new Requests class. 4. Generalise streaming response event FSM into a separate ResponseEventListener class. 5. Update StyxBackendServiceClient.

components/api/src/main/java/com/hotels/styx/api/Eventual.java

components/api/src/main/java/com/hotels/styx/api/StreamingHttpMessage.java

kvosper · 2018-10-11T14:39:44Z

components/api/src/main/java/com/hotels/styx/api/StreamingHttpMessage.java

+                });
+    }
+
+    default void drop() {


Shouldn't this call drop on the body?

No because the drop on the message still consumes the byte stream arriving from the TCP connection. It just drops each byte chunk that is received while retaining the publisher chain between styx server and styx client.

This message on the other hand drops the message. It is saying that we want to drop this message as it is and we are not interested in consuming the content.

As a result the underlying TCP connection gets closed.

So this method must be used before any of the content is read?

Let's remove this from the API for now, and make a note of this issue.

Basically the only way to "read" content before calling this method is to subscribe with a 3rd partly subscriber like so:

flux = Flux.from(response.body()) .subscribe(buf -> doSomething(buf))

So now it would be illegal to call drop while this subscription is ongoing. This is because it causes a 2nd subscription to the live data stream which is not supported.

components/api/src/test/java/com/hotels/styx/api/EventualTest.java

kvosper · 2018-10-12T09:04:14Z

components/client/src/test/unit/java/com/hotels/styx/client/TransportTest.java

    }

    @Test
-    public void releasesIfRequestIsUnsubscribedAfterHeaders() {
+    public void returnsConnectionBackToPool_delayedResponseError() {


Another test that does not follow naming conventions.

We should discuss and clarify our naming conventions. Could you clarify what aspect doesn't conform to naming conventions?

In the meanwhile I will rename this test to: returnsConnectionBackToPoolDueToCancelledHeaders.

Java identifiers use camel case only, not underscores (except in static final fields, AKA constants). This is not specific to us, just a common Java convention we follow.

kvosper · 2018-10-12T09:08:32Z

components/client/src/test/unit/java/com/hotels/styx/client/TransportTest.java

        verify(pool).closeConnection(any(Connection.class));
    }

+    @Test
+    public void terminatesConnection_bodyIsUnsubscribed() {


Will rename to returnsConnectionBackToPoolDueToDelayedResponseError.

kvosper · 2018-10-12T09:19:45Z

components/common/src/main/java/com/hotels/styx/api/ResponseEventListener.java

+    private Runnable onCompletedAction;
+
+    private final StateMachine<State> fsm = new StateMachine.Builder<State>()
+            .initialState(State.INITIAL)


These State instances can be statically imported to make the code more readable.

kvosper · 2018-10-12T09:29:47Z

components/common/src/main/java/com/hotels/styx/api/ResponseEventListener.java

+    }
+
+    public ResponseEventListener whenCancelled(Runnable action) {
+        this.cancelAction = action;


These "when" methods should do null checks.

kvosper · 2018-10-12T09:35:36Z

components/common/src/main/java/com/hotels/styx/api/ResponseEventListener.java

+            .onInappropriateEvent((state, event) -> state)
+            .build();
+
+    ResponseEventListener(Observable<HttpResponse> publisher) {


I see lots of things that could be private, and fields that could be final in this class, such as the constructor and in the inner classes further down. Please resolve them all.

classes.

components/common/src/test/java/com/hotels/styx/api/RequestsTest.java

components/common/src/test/java/com/hotels/styx/api/ResponseEventListenerTest.java

components/proxy/src/main/java/com/hotels/styx/routing/handlers/StandardHttpPipeline.java

system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/OriginCancellationMetrics.scala

system-tests/example-styx-plugin/src/main/java/loadtest/plugins/AsyncRequestPluginFactory.java

components/common/src/main/java/com/hotels/styx/api/ResponseEventListener.java

dvlato · 2018-10-16T10:20:41Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/StandardHttpPipeline.java

                }
            }
-            return fromRxObservable(toRxObservable(client.handle(request, this.context))
-                    .compose(StandardHttpPipeline::sendErrorOnDoubleSubscription));
+            return new Eventual<>(toPublisher(RxReactiveStreams.toObservable(client.handle(request, this.context))


Am I reading this correctly? You are transforming from publisher to observable and then back to publisher?

It first turns the Eventual from client.handle into an rx.Observable, so that sendErrorOnDoubleSubscription can be composed in. After that the composed rx.Observable is converted back to publisher.

This needs to be tidied up but that could be a separate enhancement.

It could be written line this, to show which parts happen when:

return new Eventual<>( toPublisher( toObservable(client.handle(request, this.context)) .compose(StandardHttpPipeline::sendErrorOnDoubleSubscription) ) );

...nents/proxy/src/test/scala/com/hotels/styx/routing/interceptors/RewriteInterceptorSpec.scala

components/api/src/main/java/com/hotels/styx/api/StreamingHttpMessage.java

components/api/src/test/java/com/hotels/styx/api/EventualTest.java

1. Styx Future: Replace StyxObservable with Eventual.

9eaa4f4

2. Transport: Add FSM to drive the connection pool returns and cancellations. 3. Add new Requests class. 4. Generalise streaming response event FSM into a separate ResponseEventListener class. 5. Update StyxBackendServiceClient.

mikkokar requested review from kvosper, dvlato and VivianLopes October 11, 2018 12:31

Fix ResponseEventListener.

ba00b9a

mikkokar force-pushed the styx-future branch from c797f85 to ba00b9a Compare October 11, 2018 15:10

kvosper suggested changes Oct 12, 2018

View reviewed changes

Improve Java API docs for ByteStream, Eventual and StreamingHttpMessage

4d35fb5

classes.

kvosper suggested changes Oct 12, 2018

View reviewed changes

mikkokar added 6 commits October 12, 2018 13:05

Address code review comments.

b460624

Eventual Unit Tests.

3f931c1

Cancellation metrics spec.

ed3a0e8

Remove drop method.

bbbc0f5

Tidy up.

5114064

Add unit test for StreamingHttpMessage.consume.

106dae2

dvlato approved these changes Oct 16, 2018

View reviewed changes

kvosper reviewed Oct 16, 2018

View reviewed changes

components/api/src/test/java/com/hotels/styx/api/EventualTest.java Outdated Show resolved Hide resolved

kvosper approved these changes Oct 16, 2018

View reviewed changes

Tidy up.

a40924c

mikkokar changed the base branch from master to feature-styx-1.0-api October 18, 2018 07:59

mikkokar merged commit c8b581f into ExpediaGroup:feature-styx-1.0-api Oct 18, 2018

mikkokar deleted the styx-future branch October 18, 2018 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Styx Eventual class for asynchronous events. #309

Styx Eventual class for asynchronous events. #309

mikkokar commented Oct 11, 2018

kvosper Oct 11, 2018

mikkokar Oct 12, 2018

kvosper Oct 12, 2018

mikkokar Oct 16, 2018

kvosper Oct 12, 2018

mikkokar Oct 16, 2018

kvosper Oct 16, 2018 •

edited

Loading

kvosper Oct 12, 2018

mikkokar Oct 16, 2018

kvosper Oct 12, 2018

kvosper Oct 12, 2018

kvosper Oct 12, 2018

dvlato Oct 16, 2018

mikkokar Oct 16, 2018

kvosper Oct 16, 2018

Styx Eventual class for asynchronous events. #309

Styx Eventual class for asynchronous events. #309

Conversation

mikkokar commented Oct 11, 2018

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

kvosper Oct 16, 2018 • edited Loading

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

kvosper Oct 16, 2018 •

edited

Loading