Add the facility to override the request timeout per include and route. #28

AndreasKl · 2018-03-11T23:31:27Z

No description provided.

thovid

Thanks for the work! Three things seem important to me:

I could not find the code that uses the timeout configured at the route - maybe I missed it...
One test seems to not assert anything
In an equals() method you compare Optionals with ==

Could you please fix these, (and, if you like, consider the other comments). Thanks!

thovid · 2018-03-12T06:51:25Z

src/main/java/com/rewedigital/composer/composing/FetchContext.java

+    private FetchContext(final String path, final String fallback, final Optional<Duration> ttl) {
+        this.path = path;
+        this.fallback = fallback;
+        this.ttl = requireNonNull(ttl);


Why requireNonNull for ttl but not for path or fallback? I really start to feel we need to find some consistent rule for these checks...

Path and fallback is used as if it is valid to have null in there...

I agree. But than, maybe Optional is better for both. I would like to have a codebase where null is always a mistake ;-)

I'll open an issue to discuss this kind of global code style questions, ok?

thovid · 2018-03-12T06:53:37Z

src/main/java/com/rewedigital/composer/composing/IncludeMarkupHandler.java

@@ -41,6 +41,10 @@ private ContentRange contentRange() {
        return contentMarkupHandler.assets();
    }

+    List<IncludedService> includedServices() {


As this is only accessible for testing, maybe add the @VisibleForTesting annotation? We have not discussed whether we want to use this annotation, but I'd like to try it to see if it improves readability

I thought about this, however we have three VisibleForTesting on the classpath, and none is from a dependency that is referenced in our pom directly. I would prefer to add my own annotation?

I agree and apologize, because I used one of the three in my last PR. Should we add our own dependency to whatever small annotation lib provides this annotation? I don't think we should implement our own annotation

Used VisibleForTesting from guava for now.

thovid · 2018-03-12T06:55:22Z

src/main/java/com/rewedigital/composer/composing/ValidatingContentFetcher.java

-        final CompositionStep step) {
-        if (path == null || path.trim().isEmpty()) {
+    public CompletableFuture<Response<String>> fetch(final FetchContext context, final CompositionStep step) {
+        if (context.path() == null || context.path().trim().isEmpty()) {


Maybe add a method like FetchContext.hasEmptyPath() or something?

Totally agree, btw. this is the cause why I did not prevent null for path and fallback.

thovid · 2018-03-12T06:56:55Z

src/main/java/com/rewedigital/composer/composing/ValidatingContentFetcher.java

            .toCompletableFuture();
    }

+    private Request withTtl(final Request request, final FetchContext context) {
+        if (context.ttl().isPresent()) {


I like context.ttl().map(ttl -> request.withTtl(ttl)).orElse(request) more than the if construct

Imho it is taste... changed.

thovid · 2018-03-12T06:57:59Z

src/main/java/com/rewedigital/composer/routing/Match.java

        final Match other = (Match) obj;
-        return Objects.equals(backend, other.backend) && routeType == other.routeType;
+        return Objects.equals(backend, other.backend) && routeType == other.routeType && ttl == other.ttl;


I think you want to use Objects.equals(ttl, other.ttl) instead of ==

thovid · 2018-03-12T07:34:53Z

src/test/java/com/rewedigital/composer/routing/RoutingConfigurationTest.java

+        assertThat(localRules).anySatisfy(rule -> {
+            final Match routeMatchWithTtl =
+                Match.of("https://target.service/{arg}", Optional.of(ofMillis(200)), PROXY);
+            assertThat(routeMatchWithTtl);


I think here is some actual assertion missing?

thovid · 2018-03-12T07:36:06Z

src/test/java/com/rewedigital/composer/routing/RoutingConfigurationTest.java

+        final List<Rule<Match>> localRules = configuration.localRules();
+        assertThat(localRules).anySatisfy(rule -> {
+            final Match routeMatchWithTtl =
+                Match.of("https://target.service/{arg}", Optional.of(ofMillis(200)), PROXY);


I think i don't like the static import of Duration.ofMillis, because Duration.ofMills(200) makes more sense to me than just ofMillis(200)

thovid · 2018-03-12T07:37:57Z

src/main/java/com/rewedigital/composer/routing/Match.java

    }

    public String backend() {
        return backend;
    }

+    public Optional<Duration> ttl() {


Might be me not understanding the coverage report, but it looks like this new method is not covered by any test?

thovid · 2018-03-12T07:45:00Z

src/main/java/com/rewedigital/composer/routing/RoutingConfiguration.java

    }

    public List<Rule<Match>> localRules() {
        return localRoutes;
    }

+    private static Optional<Duration> optionalDuration(final Config config, final String path) {


You could use com.spotify.apollo.environment.ConfigUtil here instead

That utility is private and final

On my classpath it is public final class ConfigUtil , from dependency com/spotify/apollo-environment/1.6.3/apollo-environment-1.6.3.jar

My fault, I do not really remember why I rolled my own impl.

There is no version for Duration, therefore I rolled my own impl.

thovid · 2018-03-12T07:49:12Z

src/main/java/com/rewedigital/composer/composing/IncludedService.java

+        final String unparsedValue = attributes.get(name);
+        try {
+            return Optional.of(Long.parseLong(unparsedValue));
+        } catch (final NumberFormatException nfEx) {


Maybe add a test showing what happens when the data type is wrong?

AndreasKl · 2018-03-12T08:29:03Z

Todo: Add E2E test.

thovid · 2018-03-12T22:01:51Z

Thanks for the rework!
Added issue #29 for the discussion about null vs optional and #30 for the discussion about timeout vs ttl. I'm merging the PR even though the end-2-end test is missing. I'm not sure if we should add one for something like request timeouts (ugly to test anyways ;-) ).

thovid · 2018-03-12T22:03:50Z

Ah darn, right after merging this: Could you update the readme.md to document the new features?

AndreasKl added 4 commits March 11, 2018 00:30

Started to add a request ttl per content and per route.

961a9be

Removed duplicate test.

cf4557b

Support ttl on a request basis.

dd15e6e

Add test for IncludeMarkupHandler.

7d923d7

AndreasKl requested a review from thovid March 11, 2018 23:31

thovid suggested changes Mar 12, 2018

View reviewed changes

AndreasKl added 2 commits March 12, 2018 22:04

First batch of changes based on code review.

edc5874

Fixed dependecy cycles.

3844936

thovid merged commit 7e73fe5 into rewe-digital:master Mar 12, 2018

thovid mentioned this pull request Mar 13, 2018

Add configurable time-outs for template, proxy and include routes #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the facility to override the request timeout per include and route. #28

Add the facility to override the request timeout per include and route. #28

AndreasKl commented Mar 11, 2018

thovid left a comment

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

thovid Mar 12, 2018

thovid Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018 •

edited

Loading

AndreasKl Mar 12, 2018

AndreasKl Mar 12, 2018

thovid Mar 12, 2018

AndreasKl commented Mar 12, 2018

thovid commented Mar 12, 2018

thovid commented Mar 12, 2018

Add the facility to override the request timeout per include and route. #28

Add the facility to override the request timeout per include and route. #28

Conversation

AndreasKl commented Mar 11, 2018

thovid left a 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

thovid Mar 12, 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

AndreasKl commented Mar 12, 2018

thovid commented Mar 12, 2018

thovid commented Mar 12, 2018

thovid Mar 12, 2018 •

edited

Loading