New 1.0 API: Address TODO items. #204

kvosper · 2018-07-02T16:32:10Z

Some of the TODOs have been resolved. Some have been transformed into GitHub issues. Others have requests for clarity added.

mikkokar · 2018-07-04T05:45:28Z

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

-        // This method is bad because it just takes string instead of stream of chunks. Also it implicitly
-        // assumes UTF_8 encoding.
-
+        // TODO see https://github.com/HotelsDotCom/styx/issues/200


The comment can be removed as I have merged your PR #205.

mikkokar · 2018-07-04T05:55:34Z

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

+                });
+
+        return future;
+    }


Just a note:

We should consider if the StreamingHttpMessage.java is necessary at all. Looking at this now it feels bit unnecessary interface. For example releaseContentBuffers could have been delegated if we didn't have this interface already in place.

mikkokar · 2018-07-04T06:20:02Z

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

@@ -38,6 +38,8 @@

    // TODO: Mikko: Styx 2.0 Api: `onError`: is more flexible type signature possible? Such as:
    //       <U> StyxObservable<U> onError(Function<Throwable, StyxObservable<U>> errorHandler);
+    // Doesn't look like rx.Observable.onError has a suitable method
+    // Might be possible to hack it, but it's possible that the compiler may get confused by such a method


Ok. I think we can just remove all these comments.

mikkokar · 2018-07-04T06:22:15Z

components/api/src/main/java/com/hotels/styx/api/messages/CustomHttpResponseStatus.java

-package com.hotels.styx.api.netty;
-
-import com.hotels.styx.api.messages.HttpResponseStatus;
+package com.hotels.styx.api.messages;

 /**
 * Custom HTTP response status codes.


We need to remove the netty package from the API. Can you raise another GitHub issue for this please?

Raised #207

mikkokar · 2018-07-04T06:22:50Z

components/api/src/main/java/com/hotels/styx/api/messages/HttpResponseStatus.java

-    // TODO: Mikko: Styx 2.0 API: Had to relax visibility due to CustomHttpResponseStatus class.
-    // Check with Kyle.
-    public HttpResponseStatus(int code, String description) {
+    HttpResponseStatus(int code, String description) {


mikkokar · 2018-07-04T06:52:53Z

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

@@ -32,6 +32,7 @@ public static HttpServer createAdminServer(StyxServerComponents config) {
        // TODO: Pass all backend Service Registries to AdminServerBuilder:
        // - only one backendServicesRegistry is passed in to the admin interface. Instead we
        //   should pass all of them:
+        // The comment seems to refer to part of a larger piece of work. Is there an issue to associate this with?


Haha this is your own comment @kvosper: Styx server refactoring (#116).

If you no longer remember please remove.

As the first line of the comment says, I only moved the comment here from StyxServer. I don't believe I wrote it originally. If it doesn't mean anything to you either, I will remove it.

Ok. Please remove.

mikkokar · 2018-07-04T06:54:51Z

components/server/src/main/antlr4/com/hotels/styx/server/routing/Condition.g4

@@ -50,6 +50,7 @@ DQUOTE : ["] ;
 ID : [a-zA-Z0-9_]+ ;

 // TODO: should also accept any visible non-delimiting US-ASCII character
+// Not clear what this comment is referring to. Need more information.


Let's stick to 1.0 API related TODO items. This is not one of them. Please back off this change.

mikkokar · 2018-07-04T06:57:14Z

components/server/src/main/java/com/hotels/styx/server/routing/routes/ProxyToBackendRoute.java

@@ -42,6 +42,7 @@ public static ProxyToBackendRoute proxyToBackend(HttpClient client) {
    @Override
    public StyxObservable<HttpResponse> handle(HttpRequest request, HttpInterceptor.Context context) {
        // TODO: Mikko: Styx 2.0 API:
+        // Unclear comment
        return fromRxObservable(client.sendRequest(request));


The comment was there to remind me that the client is still using bare rx observables. It can be removed now as the client interface is a well-understood limitation in the 1.0 API.

mikkokar · 2018-07-04T07:02:54Z

system-tests/performance/scripts/load-test-tool/load_test.py

@@ -49,7 +49,7 @@

 def run_command(command):
    cmd_process = subprocess.Popen(shlex.split(command), stdout=subprocess.PIPE)
-    # TODO, should read input using communicate() method:
+    # TODO, should read input using communicate() method: - what does this mean? needs looking at by someone who knows python
    cmd_process.wait()


As it is not related to the API work please back off this change.

mikkokar · 2018-07-04T07:03:19Z

system-tests/performance/scripts/load-test-tool/wrk.py

@@ -22,7 +22,7 @@

 def run_command(command):
    cmd_process = subprocess.Popen(shlex.split(command), stdout=subprocess.PIPE)
-    # TODO, should read input using communicate() method:
+    # TODO, should read input using communicate() method:  - what does this mean? needs looking at by someone who knows python


Also here, please back off this change as it is not an API related comment.

mikkokar · 2018-07-06T09:05:22Z

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

@@ -34,6 +34,7 @@ public void add(String key, Object value) {
    }

    // TODO: Mikko: Styx 2.0 API: MockObservable support for `onError`.
+    // This class is never used.
    static class MockObservable<T> implements StyxObservable<T> {


mikkokar · 2018-07-06T09:05:52Z

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

@@ -144,6 +144,7 @@ public void onChange(Registry.Changes<BackendService> changes) {
            };

            //TODO: origins inventory builder assumes that appId/originId tuple is unique and it will fail on metrics registration.
+            // We have plans to entirely revamp the OriginsInventory, see https://github.com/HotelsDotCom/styx/issues/197
            OriginsInventory inventory = new OriginsInventory.Builder(backendService.id())


Please remove the comments.

mikkokar · 2018-07-06T09:06:09Z

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

@@ -40,6 +40,7 @@ public RouteHandlerAdapter(HttpRouter router) {
                // TODO: NoServiceConfiguredException happens *after* routing. Therefore it doesn't contain
                // any helpful information about the state of the router as to why service was not configured.
                // It might be useful to think if there is a better way of addressing this issue.
+                // What kind of information might we want?


This needed to be removed, too.

mikkokar · 2018-07-06T09:06:25Z

components/proxy/src/main/java/com/hotels/styx/proxy/plugin/InstrumentedPlugin.java

@@ -89,6 +89,7 @@ public void styxStopping() {
        try {
            // TODO: Mikko: Styx 2.0 API:
            // Check this:
+            // Check what about it?


Don't forget this.

mikkokar changed the title ~~Todos~~ New 1.0 API: Address TODO items. Jul 3, 2018

mikkokar added styx-1.0 enhancement labels Jul 3, 2018

mikkokar suggested changes Jul 4, 2018

View reviewed changes

kvosper added 2 commits July 4, 2018 14:42

Fix TODOs relevant to 1.0 API

b27858b

Remove comment

454c8fa

mikkokar suggested changes Jul 6, 2018

View reviewed changes

kvosper added 2 commits July 6, 2018 13:18

Address PR comments

83eafd9

Remove unused class

55f4786

mikkokar approved these changes Jul 6, 2018

View reviewed changes

mikkokar merged commit 6a023ca into ExpediaGroup:styx-1.0-dev Jul 6, 2018

kvosper deleted the todos branch July 6, 2018 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New 1.0 API: Address TODO items. #204

New 1.0 API: Address TODO items. #204

kvosper commented Jul 2, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

kvosper Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

kvosper Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 4, 2018

mikkokar Jul 6, 2018

mikkokar Jul 6, 2018

mikkokar Jul 6, 2018

mikkokar Jul 6, 2018

New 1.0 API: Address TODO items. #204

New 1.0 API: Address TODO items. #204

Conversation

kvosper commented Jul 2, 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