Update plugin interceptor examples. #173

mikkokar · 2018-05-25T10:49:00Z

Bring plugins-scenarios.md up-to-date with new 1.0 API.

Remove unused imports.

kvosper · 2018-05-29T10:12:40Z

docs/developer-guide/plugins-scenarios.md

-                            .header("X-Foo", "Bar")
-                            .build());
-        }
+Transforming request object synchronously is trivial. Just call `request.newBuilder()` to


Change

Transforming request object

to

Transforming a request object

It might also be worth noting that "synchronously" means "in the same thread" here, rather than having to synchronise something.

kvosper · 2018-05-29T10:14:44Z

docs/developer-guide/plugins-scenarios.md

-        }
+Transforming request object synchronously is trivial. Just call `request.newBuilder()` to
+create a new `HttpRequest.Builder` object. It has already been initialised with the copy of
+the original `request`. Modify the builder as desired, consturct a new version, and pass


Typo:

consturct

kvosper · 2018-05-29T10:19:56Z

docs/developer-guide/plugins-scenarios.md

-	}
-
+This example demonstrates how to synchronously transform a HTTP response. We will
+use a `StyxObservable` `map` method to add an "X-Foo" header to the response.


a StyxObservable map method

I think this should be

the StyxObservable.map method

kvosper · 2018-05-29T10:23:35Z

docs/developer-guide/plugins-scenarios.md

-            return requestAccessQueryAsync(request)
-                .flatMap((allowed) -> allowed ? chain.proceed(request) : Observable.just(rejected));
-        }
+Sometimes it is necessary to transform the request asynchronously. For example, may need to 


For example, may need to

may be

For example, you may need to

kvosper · 2018-05-29T10:24:08Z

docs/developer-guide/plugins-scenarios.md

-                .flatMap((allowed) -> allowed ? chain.proceed(request) : Observable.just(rejected));
-        }
+Sometimes it is necessary to transform the request asynchronously. For example, may need to 
+look up external key value stores to parametrise the request transformation. The example below


Typo

parametrise

should be

parameterise

It appears to be a valid alternative spelling for "parameterise". But I have changed regardless.

kvosper · 2018-05-29T11:22:32Z

docs/developer-guide/plugins-scenarios.md

-business domain object. Here we will explain how to do this. And as always, this can be done in a synchronous or 
-asynchronous fashion.
+    private static CompletableFuture<String> callTo3rdParty(String myHeader) {
+        return CompletableFuture.completedFuture("value");


To reduce confusion we could just put something like

private static CompletableFuture<String> callTo3rdParty(String myHeader) { // make call here }

instead of a dummy implementation

kvosper · 2018-05-29T11:24:14Z

docs/developer-guide/plugins-scenarios.md

+                .flatMap(response ->                                                                             // (3)
+                        StyxObservable
+                                .from(callTo3rdParty(response.header(X_MY_HEADER).orElse("default")))            // (1)
+                                .map(value ->                                                                    // (4)


Maybe rename value to resultOf3rdPartyCall or something.

Not sure if long variable names make lambda expressions any clearer.

Length was not my goal, just meaning. But this is pretty minor so feel free to leave it as it is.

kvosper · 2018-05-29T11:31:46Z

...client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java

@@ -106,6 +107,8 @@ private synchronized void bootstrap(Connection.Settings connectionSettings) {
        protected void initChannel(Channel ch) {
            ChannelPipeline pipeline = ch.pipeline();

+            pipeline.addLast("logging-handler-1", new LoggingHandler());


Why -1 and -2? The names should explain why we need two.

Were these pushed accidentally?

Added some instrumentation hoping to replicate a CI test failure.

Backing this off.

kvosper · 2018-05-29T11:32:11Z

...src/main/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagator.java

@@ -136,9 +136,12 @@ private void scheduleResourcesTearDown(ChannelHandlerContext ctx) {
    protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Exception {
        ensureContentProducerIsCreated(ctx);

+        LOGGER.warn("channelRead0: {}", msg);


Was this pushed accidentally?

kvosper · 2018-05-29T11:42:46Z

docs/developer-guide/plugins-scenarios.md

-as releasing them if necessary.
+Alternatively, streaming messages can be aggregated into a `FullHttpRequest` or `FullHttpResponse` 
+messages. The full HTTP message body is then available at interceptor's disposal. Note that content
+aggregation is always an asynchronous operation. This is because Styx must wait until all content


Wouldn't it be better to say "content aggregation must be performed asynchronously to prevent blocking of the I/O thread" or something?

No. This is because content aggregation will happen asynchronously anyway, and therefore it won't be blocking a thread.

The real reason why the aggregation has to happen asynchronously is because the content simply won't be available in full at the time of aggregation. Styx simply has to wait until the underlying stream produces the necessary content.

Well, one could say that it "must be performed asynchronously to prevent blocking the IO thread", but that is missing the point, really.

I have attempted to clarify this:

Styx exposes HTTP messages to interceptors as streaming `HttpRequest` and `HttpResponse` messages. In this form, the interceptors can process the content in a streaming fashion. That is, they they can look into, and modify the content as it streams through. Alternatively, streaming messages can be aggregated into a `FullHttpRequest` or `FullHttpResponse` messages. The full HTTP message body is then available at interceptor's disposal. Note that content aggregation is always an asynchronous operation. This is because the streaming HTTP message is exposing the content, in byte buffers, as it arrives from the network, and Styx must wait until all content has been received.

What do you think?

dvlato

It seems that this PR needs some clean up to remove code that was added during troubleshooting.

kvosper · 2018-05-30T16:22:58Z

docs/developer-guide/plugins-scenarios.md

-a business domain object of type T. Styx will take care of managing all aspects related to direct memory buffers, such 
-as releasing them if necessary.
+Alternatively, streaming messages can be aggregated into a `FullHttpRequest` or `FullHttpResponse` 
+messages. The full HTTP message body is then available at interceptor's disposal. Note that content


I would change

The full HTTP message body is then available at interceptor's disposal.

to

The full HTTP message body is then available for the interceptor to use.

Of course. It reads better that way.

mikkokar added 5 commits May 24, 2018 11:28

Add request/response transformation examples into plugin-scenarios.md.

d9c133f

Update plugin-scenarios.md with 1.0 API.

5ee7243

Add some words of warning to the plugin-scenarios.md.

3d23368

Add instrumentation.

30b0a43

Add instrumentation to ProxySpec.

867add0

Remove unused imports.

mikkokar force-pushed the update-devdocs-2 branch from c75fae1 to 867add0 Compare May 29, 2018 08:08

kvosper suggested changes May 29, 2018

View reviewed changes

dvlato reviewed May 30, 2018

View reviewed changes

mikkokar added 2 commits May 30, 2018 17:17

Fix typos and remove unnecessary code changes.

d23b7cf

Back off unnecessary instrumentation.

0f6e1af

kvosper reviewed May 30, 2018

View reviewed changes

Paraphrase plugin-scenarios.md document.

923a72d

kvosper approved these changes May 30, 2018

View reviewed changes

mikkokar merged commit ad95aae into ExpediaGroup:styx-1.0-dev May 30, 2018

mikkokar deleted the update-devdocs-2 branch June 27, 2018 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update plugin interceptor examples. #173

Update plugin interceptor examples. #173

mikkokar commented May 25, 2018

kvosper May 29, 2018

kvosper May 29, 2018

kvosper May 29, 2018

kvosper May 29, 2018

kvosper May 29, 2018

mikkokar May 30, 2018

kvosper May 29, 2018

kvosper May 29, 2018

mikkokar May 30, 2018

kvosper May 30, 2018

kvosper May 29, 2018

kvosper May 29, 2018

mikkokar May 30, 2018

kvosper May 29, 2018

mikkokar May 30, 2018

kvosper May 29, 2018

mikkokar May 30, 2018

dvlato left a comment

kvosper May 30, 2018

mikkokar May 30, 2018

Update plugin interceptor examples. #173

Update plugin interceptor examples. #173

Conversation

mikkokar commented May 25, 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

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

dvlato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment