Decouple admin server from plugins startup, add proxy status endpoint #181

kvosper · 2018-06-11T16:27:43Z

No description provided.

dvlato · 2018-06-11T16:56:35Z

components/common/src/main/java/com/hotels/styx/common/Result.java

+ *
+ * @param <T> type of success value
+ */
+public class Result<T> {


I haven't checked the code that uses this class or thought about it considering we are doing Java, but I believe we wanted to implement this as an Either/Try monad? So I would expect : map, flatMap... Scala's try monad is nicer (as in simpler) https://mauricio.github.io/2014/02/17/scala-either-try-and-the-m-word.html , but it seems there are a few implementations in Java such as https://github.com/jasongoodwin/better-java-monads/blob/master/src/main/java/com/jasongoodwin/monads/Try.java

The goal of this class was simply to add a representation of success or failure that can be stored as a value.

I can see a similarity with the Try.java linked, but I'm not sure it is worth adding a new dependency just for this class.

In terms of API, a difference I see is that the Result class can be instantiated directly with a call to success or failure without needing to wrap a block of code, so it is in that sense more decoupled from the try-catch structure and can be used in situations where try-catch does not appear (e.g. using result values in unit tests).

mikkokar · 2018-06-12T12:33:13Z

components/common/src/test/java/com/hotels/styx/configstore/ConfigStoreTest.java

+    }
+
+    @Test
+    public void valueInConfigStoreIsConsistentWithListenersUpdate() {


This test can be folded into the previous one (listenersReceiveUpdatesWhenValuesChange) by adding one line at the end:

assertThat(configStore.get("foo"), is(of("bar")));

That is:

@Test public void valueInConfigStoreIsConsistentWithListenersUpdate() { AtomicReference<String> update = new AtomicReference<>(); Latch sync = new Latch(1); configStore.watch("foo", String.class) .subscribe(value -> { update.set(value); sync.countDown(); }); configStore.set("foo", "bar"); sync.await(1, SECONDS); assertThat(update.get(), is("bar")); assertThat(configStore.get("foo"), is(of("bar"))); }

mikkokar · 2018-06-12T12:42:34Z

components/common/src/test/java/com/hotels/styx/configstore/ConfigStoreTest.java

+
+        configStore.set("foo", "bar");
+        unlockedByTestThread.countDown();
+        unlockedBySubscribeThread.await(2, SECONDS);


Can you clarify the purpose of this test please? Is it to show that the watch subscriptions run from a different thread from the config setter thread?

If so, does every watch subscriber run in their own thread? How far will it scale? Say I have say 10 watch subscribes on "foo", then on setting "foo" do they run simultaneously on 10 different threads? Or are they queued to a thread pool?

Is it to show that the watch subscriptions run from a different thread from the config setter thread?

Yes.

If so, does every watch subscriber run in their own thread? How far will it scale? Say I have say 10 watch subscribes on "foo", then on setting "foo" do they run simultaneously on 10 different threads? Or are they queued to a thread pool?

That code is already part of this PR. You can look at the watch method in the ConfigStore class. Please review.

Ok this was just to probe your intentions regarding config store threading model. I think it is ok for now. Perhaps in future we may want to allow consumers to provide a custom executor.

mikkokar · 2018-06-12T14:17:07Z

components/common/src/test/java/com/hotels/styx/configstore/ConfigStoreTest.java

+                .subscribe(value -> {
+                    unlockedByTestThread.await(1, SECONDS);
+                    unlockedBySubscribeThread.countDown();
+                });


When registering a watch, a possibly existing value should be propagated straight away. This prevents unnecessary race conditions where a consumer is late to observe the event.

Please confirm if the watch behaves like this?

This is not currently the case, I will fix it.

mikkokar · 2018-06-12T14:26:04Z

components/proxy/src/main/java/com/hotels/styx/admin/handlers/ProxyStatusHandler.java

+                .map(Result::successValue)
+                .filter(Optional::isPresent)
+                .map(Optional::get)
+                .orElse("INCOMPLETE");


I wouldn't store anything complicated in the "server.started.proxy" attribute. Please consider just storing an enum of STARTED, FAILED or INCOMPLETE instead of Result

Also, a caused exception is stored in the result object (failure(cause)) in NettyServer java, just to be thrown away from the server status response.

Therefore consider removing Result class.

The detailed error cause can be logged.

Also it is going to eliminate the above 6 or 7 lines of code :-)

mikkokar · 2018-06-12T15:08:23Z

components/proxy/src/main/java/com/hotels/styx/admin/AdminServerBuilder.java

+                    routesForPlugins(plugins).forEach(route -> httpRouter.add(route.path(), route.handler()));
+                    httpRouter.add("/admin/tasks/plugin/", new PluginToggleHandler(plugins));
+                    httpRouter.add("/admin/plugins", new PluginListHandler(plugins));
+                });


We don't necessarily need a watch functionality for this use case. You can just render the admin links in handle() method every time a request comes in. This way you'll just query the config store and add the links to the end of the page.

This code does not render links, it builds the HttpRouter.

mikkokar · 2018-06-12T15:13:41Z

components/proxy/src/main/java/com/hotels/styx/admin/AdminServerBuilder.java

+        }
+    }
+
+    private static void verifyThatLinksInIndexMapToRealEndpoints(Iterable<IndexHandler.Link> links, Set<String> paths) {


Isn't this something we should have in the unit/e2e tests, rather than in the running code?

What would be the suggested remedial action for the end user if this exception ever fires?

mikkokar · 2018-06-12T15:18:52Z

components/proxy/src/main/java/com/hotels/styx/proxy/ProxyServerBuilder.java

                .build();
    }

    private HttpResponse.Builder addInfoHeader(HttpResponse.Builder responseBuilder, HttpRequest request) {
        return responseBuilder.header(styxInfoHeaderName, responseInfoFormat.format(request));
    }

-    public ProxyServerBuilder httpHandler(HttpHandler2 httpHandler) {
+    public ProxyServerBuilder httpHandler(Supplier<HttpHandler2> httpHandler) {
        this.httpHandler = httpHandler;
        return this;


Can you remind me why the httpHnadler has to be loaded lazily? To prevent a blocking load of plugins before the admin interface starts?

Yes that is the reason.

mikkokar · 2018-06-12T15:24:20Z

components/proxy/src/main/java/com/hotels/styx/startup/ProxyServerSetUp.java

@@ -66,18 +66,20 @@ private static void initialisePlugins(Iterable<NamedPlugin> plugins) {
        if (exceptions > 0) {
            throw new RuntimeException(format("%s plugins failed to start", exceptions));
        }
+
+        configStore.set("plugins", plugins);


Ideally this would move inside the loop:

for (NamedPlugin plugin : plugins) { try { plugin.styxStarting(); configStore.set("plugins", pluginsInitialisedSoFar); // <---- } catch (Exception e) { exceptions++; LOG.error("Error starting plugin '{}'", plugin.name(), e); } }

This would give a more instantaneous feedback about the plugins loaded so far. The end user could monitor the progress of plugins being initialised. Maybe something to consider in future.

However the current implementation is fine for now.

I had the same thought, but for now the current implementation is simpler.

mikkokar · 2018-06-13T08:45:43Z

The Travis build has started failing. I assume you are still working on this PR?

There are three failing e2e tests in OriginsReloadRepetitionSpec and OriginsReloadSpec test suites. All of them show similar symptoms:

�[31m  The code passed to eventually never returned normally. Attempted 60 times over 1.00299402 seconds. Last failure message: "{"appOne":{"appId":"appOne","activeOrigins":[],"inactiveOrigins":[],"disabledOrigins":[]}}" did not include substring "localhost:41693". (OriginsReloadSpec.scala:73)�[0m
�[31m- should reflect origin removal in origins status page *** FAILED ***�[0m

That is, no origins are shown in the origin status response.

kvosper · 2018-06-14T15:20:06Z

An update on failures - I cannot reproduce this failure locally, making it difficult to diagnose. I am going to try increasing timeouts on the tests in OriginsReloadSpec to see if it makes a difference. I will likely also add extra logging as I cannot see any difference in the logs when the tests pass compared to these failures.

Update for my update - increasing the timeout for should reflect origin addition in origins status page from 1 second to 10 seconds did not prevent it from failing.

kvosper · 2018-06-15T09:57:14Z

Looks like the test OriginsReloadSpec causes two OriginsSnapshots to be sent when it changes the backends. First to remove the old origins, then to add the new ones.
For some reason, on travis, the snapshot from the removal is being received later than the snapshot from the addition, i.e. out of order.

…affects the build

…e if it affects the build" This reverts commit 5332ee5.

This reverts commit 9b530a2.

… events

This reverts commit 4677991.

…to be removed later)

kvosper · 2018-06-25T09:37:27Z

This is being postpone until required improvements are made separately.

kvosper · 2018-08-06T15:54:03Z

Closing as this is dependent on other work, and the codebase has changed since it was opened. It will probably be done after Styx 1.0 is released.

kvosper added 2 commits June 11, 2018 16:46

Decouple admin server from plugins startup, add proxy status endpoint

9bb144f

Update documentation and tidy code

e8373d3

dvlato suggested changes Jun 11, 2018

View reviewed changes

mikkokar suggested changes Jun 12, 2018

View reviewed changes

Changes for review

1ac7258

mikkokar reviewed Jun 12, 2018

View reviewed changes

Changes for review

c584317

mikkokar reviewed Jun 12, 2018

View reviewed changes

kvosper added 3 commits June 14, 2018 16:02

Remove old stuff

c4bb53b

Try increasing timeouts

791be50

Fix accidental commit

1472072

Add some logging

a04bd19

kvosper added 12 commits June 18, 2018 11:51

Put actions in sequence

9b530a2

Add delay between removing origins and adding new ones, to see if it …

5332ee5

…affects the build

remove logging

4a2ff8a

Revert "Add delay between removing origins and adding new ones, to se…

eb05fef

…e if it affects the build" This reverts commit 5332ee5.

Revert "Put actions in sequence"

31eba97

This reverts commit 9b530a2.

Closing/closed OriginsInventory instances should not process any more…

2f9b104

… events

WIP

4677991

Revert "WIP"

386406e

This reverts commit 4677991.

Move eventBus.unregister to start of close

52afc8b

Add some extensive debug logging for travis test failures (will need …

e1944d5

…to be removed later)

fix mistake

3133bc8

Send GetOriginsInventorySnapshot event through eventQueue

a2a8db7

kvosper added 3 commits June 21, 2018 17:22

Remove debugging stuff

89793da

Add back log line

3c125d4

Temporarily remove logging to see if travis fails

8bb0c23

kvosper closed this Aug 6, 2018

kvosper deleted the server-started-endpoint branch August 24, 2018 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple admin server from plugins startup, add proxy status endpoint #181

Decouple admin server from plugins startup, add proxy status endpoint #181

kvosper commented Jun 11, 2018

dvlato Jun 11, 2018

kvosper Jun 12, 2018

mikkokar Jun 12, 2018

mikkokar Jun 12, 2018

kvosper Jun 12, 2018

mikkokar Jun 12, 2018

mikkokar Jun 12, 2018

kvosper Jun 12, 2018

mikkokar Jun 12, 2018

mikkokar Jun 12, 2018

mikkokar Jun 12, 2018

mikkokar Jun 12, 2018 •

edited

kvosper Jun 12, 2018

mikkokar Jun 12, 2018

mikkokar Jun 12, 2018

kvosper Jun 12, 2018

mikkokar Jun 12, 2018

kvosper Jun 12, 2018

mikkokar commented Jun 13, 2018

kvosper commented Jun 14, 2018 •

edited

kvosper commented Jun 15, 2018

kvosper commented Jun 25, 2018

kvosper commented Aug 6, 2018

Decouple admin server from plugins startup, add proxy status endpoint #181

Decouple admin server from plugins startup, add proxy status endpoint #181

Conversation

kvosper commented Jun 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

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

mikkokar Jun 12, 2018 • 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

mikkokar commented Jun 13, 2018

kvosper commented Jun 14, 2018 • edited

kvosper commented Jun 15, 2018

kvosper commented Jun 25, 2018

kvosper commented Aug 6, 2018

mikkokar Jun 12, 2018 •

edited

kvosper commented Jun 14, 2018 •

edited