Enabling event bus delivery options for communication between Server … #349

tomaszmichalak · 2017-11-28T19:17:13Z

I hereby agree to the terms of the Knot.x Contributor License Agreement.

…and RepositoryConnectors/Knots; ServiceKnot and ServiceAdatpers; ActionKnot and ActionAdapters.

toniedzwiedz · 2017-11-28T19:28:24Z

knotx-core/src/main/java/io/knotx/proxy/reactive/AdapterProxyFactory.java

+    // hidden
+  }
+
+  public static AdapterProxy createProxy(Vertx vertx, Optional<DeliveryOptions> deliveryOptions, String address) {


Not sure how this fits into the architecture of this area of Knot.x but consuming an Optional as a parameter seems a bit strange. How about overloading the method to either accept the DeliveryOptions or not?

public static AdapterProxy createProxy(Vertx vertx, String address, DeliveryOptions deliveryOptions) //... public static AdapterProxy createProxy(Vertx vertx, String address)

The caller either has the delivery options or not. If they don't, with the current API, they need to pass an empty optional here.

I can see this pattern in more places and I'm wondering what the benefits of using an Optional this way are.

@toniedzwiedz Please check what is the expected behavior in this case. We have a configuration that has a getter:
Optional<DeliveryOptions> get()
It means that we expect DeliveryOptions may be not defined. Based on this value we want to create a factory method that uses two different methods with API (with and without DeliveryOptions). I don't know what is best in this situation...

marcinczeczko · 2017-11-28T21:31:48Z

knotx-core/src/main/java/io/knotx/proxy/AdapterProxy.java

@@ -31,5 +32,9 @@ static AdapterProxy createProxy(Vertx vertx, String address) {
    return new AdapterProxyVertxEBProxy(vertx, address);
  }

+  static AdapterProxy createProxyWithOptions(Vertx vertx, String address, DeliveryOptions deliveryOptions) {


why you added the new createProxy method ? can it be just one createProxy() method consuming deliveryOptions ?
I see that you created Factory specifically for instantiation of Knot's, Adapters, etc. that uses createProxy method with or without deliveryOptions. But it's not needed at all. A ....VertxEBProxy implementation handles it for you - if you will set deliveryOptions to null it will not use it - you don't need to do any handling or factories.

I'm just thinking out loud.
@marcinczeczko don't you agree that calling:
AdapterProxy.createProxy(vertx, address, null) looks odd?
Why the null? Should this interface force the user to pass DeliveryOptions parameter?
This is just to make the code look good, and skip in my opinion weird constructions like
Factory.construct(param, null, null, param2).

@marcinczeczko You are right that Vert.x handle nulls. But as a result we will have to add nulls in all tests. So should we have AdapterProxy.createProxy(vertx, address, null) in all tests? Agree that it is "a sense of taste".

Yes, but I don't like overengineering by adding Factories to handle nulls, don't you think isn't too big for such small thing ?

After the offline catchup with @marcinczeczko:

let's not break compatibility and stay with 2 factory methods in ProxyGen helpers (like in this interface)

classes like XXXProxyFactory are a bit overengineered, let's just construct Proxies directly where it is necessary

Optional in config POJOs - suggestion here is to stick with Vert.x approach, and initialize not set properties with default values, e.g. in constructor:

... deliveryOptions = config.containsKey("deliveryOptions") ? new DeliveryOptions(config.getJsonObject("deliveryOptions")) : new DeliveryOptions(); ...

marcinczeczko · 2017-11-28T21:32:14Z

knotx-core/src/main/java/io/knotx/proxy/KnotProxy.java

@@ -30,5 +31,9 @@ static KnotProxy createProxy(Vertx vertx, String address) {
    return new KnotProxyVertxEBProxy(vertx, address);
  }

+  static KnotProxy createProxyWithOptions(Vertx vertx, String address, DeliveryOptions deliveryOptions) {


Same here as with AdapterProxy...

marcinczeczko · 2017-11-28T21:32:26Z

knotx-core/src/main/java/io/knotx/proxy/RepositoryConnectorProxy.java

@@ -31,5 +32,9 @@ static RepositoryConnectorProxy createProxy(Vertx vertx, String address) {
    return new RepositoryConnectorProxyVertxEBProxy(vertx, address);
  }

+  static RepositoryConnectorProxy createProxyWithOptions(Vertx vertx, String address, DeliveryOptions deliveryOptions) {


Same here as with AdapterProxy...

marcinczeczko · 2017-11-28T21:37:17Z

knotx-core/src/main/java/io/knotx/proxy/reactive/KnotProxyFactory.java

@@ -0,0 +1,36 @@
+/*


See my initial comment - factories are not required at all

malaskowski · 2017-11-28T21:51:13Z

knotx-knot/knotx-knot-action/src/main/java/io/knotx/knot/action/ActionKnotProxyImpl.java

@@ -102,7 +103,8 @@ private KnotContext handleGetMethod(List<FormEntity> forms, KnotContext knotCont

  private Single<AdapterResponse> callActionAdapter(KnotContext knotContext, FormEntity current) {
    LOGGER.trace("Process form for {} ", knotContext);
-    AdapterProxy adapter = AdapterProxy.createProxy(vertx, current.adapter().getAddress());
+    AdapterProxy adapter = AdapterProxyFactory


Continuing @toniedzwiedz comment, this should rather look like:

AdapterProxy adapter = configuration.getDeliveryOptions().isPresent() ? AdapterProxyFactory.createProxy(vertx, configuration.getDeliveryOptions().get(), address) : AdapterProxyFactory.createProxy(vertx, address)

malaskowski · 2017-11-29T09:34:34Z

knotx-core/src/main/java/io/knotx/proxy/AdapterProxy.java

@@ -31,5 +32,9 @@ static AdapterProxy createProxy(Vertx vertx, String address) {
    return new AdapterProxyVertxEBProxy(vertx, address);
  }

+  static AdapterProxy createProxyWithOptions(Vertx vertx, String address, DeliveryOptions deliveryOptions) {


After the offline catchup with @marcinczeczko:

let's not break compatibility and stay with 2 factory methods in ProxyGen helpers (like in this interface)

classes like XXXProxyFactory are a bit overengineered, let's just construct Proxies directly where it is necessary

Optional in config POJOs - suggestion here is to stick with Vert.x approach, and initialize not set properties with default values, e.g. in constructor:

... deliveryOptions = config.containsKey("deliveryOptions") ? new DeliveryOptions(config.getJsonObject("deliveryOptions")) : new DeliveryOptions(); ...

tomaszmichalak · 2017-11-29T13:11:19Z

CR fixes.

marcinczeczko · 2017-11-29T13:07:13Z

documentation/src/main/wiki/ActionKnot.md

+
+### Vert.x Event Bus delivery options
+
+While HTTP request processing, Action Knot calls Adapter using 


`During HTTP request processing, Knot communicates with Adapter over the Vert.x event bus. Low level aspects of that communication can be configured using Vert.x Delivery Options object. The caller of the Adapter, such as Action or Service Knot, can customise default settings by providing a deliveryOptions field in the config section of its configuration.

Comment: Can we move a description of setting up delivery option to one place ? e.g. on Knot documentation page ? it would be hard to manage doc if it's in multiple places

marcinczeczko · 2017-11-29T13:14:39Z

documentation/src/main/wiki/ActionKnot.md

+bus. It can be used to control the low level aspects of the event bus communication like timeouts, headers, message 
+codec names.
+
+The `deliveryOptions` need to be added in the following place, of the Action Knot configuration to define the 


deliveryOptions needs to be added as shown below:

NOTE: don't use Action Knot here as it can be set on any Knot

@marcinczeczko The communication takes place in three places:

Between Server and Repository Connector / other Knots.

Between Service Knot and Service Adapters.

Between Action Knot and Action Adapters.

This is the reason we define those deliveryOptions in those places. So event publisher defines the event bus rules.

marcinczeczko · 2017-11-29T13:16:30Z

documentation/src/main/wiki/Server.md

+headers, message codec names.
+
+
+The `repositoryDeliveryOptions` need to be added in the following place, of the KnotxServerVerticle configuration to define 


why it can be named deliveryOptions ??

We want to be able to specify deliveryOptions for a repository connector and knots separately.

If we want to have separate delivery options per each connected address, maybe we should have "global" deliveryOptions that defines default options for each Proxy and additionally allow to define options inside each section and keep consistent name deliveryOptions, e.g.:

{ "main": "io.knotx.server.KnotxServerVerticle", "options": { "config": { "httpPort": 8092, "displayExceptionDetails": true, "deliveryOptions": { "timeout": 15000 }, "repositories": [ { "path": "/content/local/.*", "address": "knotx.core.repository.filesystem", "deliveryOptions": { "timeout": 30000 } }, { "path": "/content/.*", "address": "knotx.core.repository.http", "deliveryOptions": { "timeout": 30000 } } ], "splitter": { "address": "knotx.core.splitter" }, "routing": { "GET": [ { "path": ".*", "address": "knotx.knot.service", "deliveryOptions": { "timeout": 60000 }, "onTransition": { "next": { "address": "knotx.knot.handlebars" } } } ] }, "assembler": { "address": "knotx.core.assembler" } } } }

In the case above, following timeouts will be like:

filesystem repository: 30s,

http repository: 30s,

knotx.knot.service 60s,

all others 15s

@Skejven Your proposal looks very interesting. I recommend to merge the current solution and investige more advanced possibilities if required in the future.

Let's then get rid of repositoryDeliveryOptions and enable setting it with name deliveryOptions either on verticle level (e.g. Server) or only on producer-level (e.g. only repositories).

marcinczeczko

I forgot to mention that two things need to be done before merging:

Update CHANGELOG.md
Update wiki UPGRADE NOTES.md - even if it does not require any action after upgrade, it's worth to mention that a delivery options can be customized with references to the documentation

toniedzwiedz reviewed Nov 28, 2017

View reviewed changes

marcinczeczko suggested changes Nov 28, 2017

View reviewed changes

malaskowski reviewed Nov 28, 2017

View reviewed changes

Documentation update.

0a6fbd6

malaskowski requested changes Nov 29, 2017

View reviewed changes

CR changes.

47eade7

marcinczeczko reviewed Nov 29, 2017

View reviewed changes

Skejven and others added 3 commits November 29, 2017 18:07

Fixed formatting

d7bdc6a

removed repository delivery options

e0728aa

merge master resolve conflicts

8ad6322

malaskowski approved these changes Dec 1, 2017

View reviewed changes

marcinczeczko approved these changes Dec 1, 2017

View reviewed changes

marcinczeczko suggested changes Dec 1, 2017

View reviewed changes

malaskowski added 2 commits December 1, 2017 12:00

Updated changelog and upgrade notes

daac554

resolved conflicts with master

f8436b7

marcinczeczko approved these changes Dec 1, 2017

View reviewed changes

malaskowski merged commit b179457 into master Dec 4, 2017

malaskowski deleted the feature/event-bus-timeouts branch December 4, 2017 07:47

malaskowski mentioned this pull request Jan 4, 2018

EventBus timeout is not configurable, current timout is 30s #343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling event bus delivery options for communication between Server … #349

Enabling event bus delivery options for communication between Server … #349

tomaszmichalak commented Nov 28, 2017

toniedzwiedz Nov 28, 2017

tomaszmichalak Nov 29, 2017

marcinczeczko Nov 28, 2017

malaskowski Nov 28, 2017

tomaszmichalak Nov 29, 2017 •

edited

Loading

marcinczeczko Nov 29, 2017

malaskowski Nov 29, 2017

tomaszmichalak Nov 29, 2017

marcinczeczko Nov 28, 2017

tomaszmichalak Nov 29, 2017

marcinczeczko Nov 28, 2017

tomaszmichalak Nov 29, 2017

marcinczeczko Nov 28, 2017

malaskowski Nov 28, 2017

malaskowski Nov 29, 2017

tomaszmichalak commented Nov 29, 2017

marcinczeczko Nov 29, 2017

tomaszmichalak Nov 29, 2017

marcinczeczko Nov 29, 2017

tomaszmichalak Nov 29, 2017

tomaszmichalak Nov 29, 2017

marcinczeczko Nov 29, 2017

tomaszmichalak Nov 29, 2017

malaskowski Nov 29, 2017

tomaszmichalak Nov 30, 2017

malaskowski Nov 30, 2017 •

edited

Loading

marcinczeczko left a comment •

edited

Loading


		### Vert.x Event Bus delivery options

		While HTTP request processing, Action Knot calls Adapter using

		headers, message codec names.


		The `repositoryDeliveryOptions` need to be added in the following place, of the KnotxServerVerticle configuration to define

Enabling event bus delivery options for communication between Server … #349

Enabling event bus delivery options for communication between Server … #349

Conversation

tomaszmichalak commented Nov 28, 2017

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

tomaszmichalak Nov 29, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaszmichalak commented Nov 29, 2017

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

malaskowski Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

marcinczeczko left a comment • edited Loading

Choose a reason for hiding this comment

tomaszmichalak Nov 29, 2017 •

edited

Loading

malaskowski Nov 30, 2017 •

edited

Loading

marcinczeczko left a comment •

edited

Loading