Refactor api commands by aahmed-se · Pull Request #3168 · apache/pulsar

aahmed-se · 2018-12-12T00:47:17Z

We are deprecating the Response Object in favor data object responses.

sijie

it is unclear to me what is this PR for. please update the description with your motivation and what are the changes.

sijie · 2018-12-12T02:07:12Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SinkBase.java

I don't understand why the return value was changed to void?

because there is no output that needs to be returned. If there is an error raising a RestException will trigger jetty to return the error message

but this is a breaking change no? How can old pulsar admin clients talk to new broker?

updateSink won't be a breaking change because the the response returned has no content just a 400 status code

sijie · 2018-12-12T02:07:48Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdSources.java

let's not couple fixing typos with other changes.

sijie · 2018-12-12T02:08:19Z

.../src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/FunctionApiV2ResourceTest.java

please don't do this.

aahmed-se · 2018-12-12T03:19:28Z

I didn't mark this ready for review yet.

srkukarni · 2018-12-13T20:35:10Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/FunctionsBase.java

This should return FunctionConfig

srkukarni · 2018-12-13T20:36:19Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/FunctionsBase.java

this should return a list

srkukarni · 2018-12-13T20:36:51Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/FunctionsBase.java

this should return something. Not sure if its String or byte[]

srkukarni · 2018-12-13T20:37:11Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/FunctionsBase.java

This should not return void

srkukarni · 2018-12-13T20:37:40Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/FunctionsBase.java

this should not return void

srkukarni · 2018-12-13T20:38:06Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SinkBase.java

return SinkConfig

srkukarni · 2018-12-13T20:38:17Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SinkBase.java

return list

srkukarni · 2018-12-13T20:38:42Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SourceBase.java

return sourceconfig

srkukarni · 2018-12-13T20:38:49Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SourceBase.java

return list

jerrypeng · 2018-12-13T20:39:36Z

pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SinkImpl.java

We should just have another method called getSinkInfo and it should return a SinkConfig not object

jerrypeng · 2018-12-13T20:39:55Z

...r-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SourceImpl.java

Same as my comment for Sink

srkukarni · 2018-12-13T20:40:31Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

Can you rename the function to throwUnavailableException

srkukarni · 2018-12-13T20:45:01Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

This should be FunctionConfig

srkukarni · 2018-12-13T20:48:25Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

this is not void

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

srkukarni · 2018-12-13T20:49:54Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

srkukarni · 2018-12-13T20:50:23Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

should return this value

jerrypeng · 2018-12-18T00:08:47Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

        when(mockedManager.containsFunction(eq(tenant), eq(namespace), eq(source))).thenReturn(false);

-        Response response = updateDefaultSource();
-        assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:08:56Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

        when(mockedManager.containsFunction(eq(tenant), eq(namespace), eq(source))).thenReturn(true);

-        Response response = updateDefaultSource();
-        assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:09:06Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

        when(mockedManager.updateFunction(any(FunctionMetaData.class))).thenReturn(requestResult);

-        Response response = updateDefaultSource();
-        assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:09:14Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

        when(mockedManager.updateFunction(any(FunctionMetaData.class))).thenReturn(requestResult);

-        Response response = updateDefaultSource();
-        assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:09:21Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

            function,
                null);

-        assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:09:29Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

        when(mockedManager.containsFunction(eq(tenant), eq(namespace), eq(source))).thenReturn(false);
-
-        Response response = deregisterDefaultSource();
-        assertEquals(Status.NOT_FOUND.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:11:39Z

...er/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2ResourceTest.java

    public void testRegisterFunctionNonexistantNamespace() throws Exception {
        this.namespaceList.clear();
-        Response response = registerDefaultSource();
-        assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:12:23Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

                new Gson().toJson(sinkConfig),
                null);

-        assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());


we should still check status code

jerrypeng · 2018-12-18T00:16:57Z

.../src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/FunctionApiV2ResourceTest.java

-        if (expectedError == null) {
-            assertEquals(Status.OK.getStatusCode(), response.getStatus());
-        } else {
-            assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus());


we should check status code

Add Status code Asserts

srkukarni · 2018-12-18T22:34:39Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

            log.error("{} does not exist @ /{}/{}/{}", componentType, tenant, namespace, componentName);
-            return Response.status(Status.NOT_FOUND).type(MediaType.APPLICATION_JSON)
-                    .entity(new ErrorData(String.format("%s %s doesn't exist", componentType, componentName))).build();
+            throw new RestException(Status.BAD_REQUEST, String.format("%s %s doesn't exist", componentType, componentName));


the return code should be not found

srkukarni · 2018-12-18T22:37:05Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java


        if (null == worker().getStateStoreAdminClient()) {
-            return getStateStoreUnvailableResponse();
+            getStateStoreUnvailableResponse();


this should be a return statement

We are throwing an exception why would there be a return value

Rename getStateStoreUnvailableResponse

jerrypeng · 2018-12-18T23:06:03Z

...unctions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java

-    public Response getFunctionInfo(final String tenant,
-                                    final String namespace,
-                                    final String componentName) {
+    public FunctionConfig getFunctionInfo(final String tenant,


This function should get moved out into org.apache.pulsar.functions.worker.rest.api.FunctionImpl

srkukarni · 2018-12-19T17:10:23Z

run integration tests

srkukarni · 2018-12-19T23:15:18Z

@sijie does this pr look good to you now?

sijie

since it is changing the rest api, please add an integration test to make sure old admin client can talk to new brokers.

if the old admin client can't talk to new brokers, it is breaking BC. then you should consider adding v3 api instead of changing the v2 api.

sijie · 2018-12-20T03:13:28Z

...worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/v2/SourceApiV2Resource.java

    @ApiResponses(value = { @ApiResponse(code = 400, message = "Invalid request"),
-            @ApiResponse(code = 404, message = "The function does not exist"),
-            @ApiResponse(code = 500, message = "Internal server error") })
+    @ApiResponse(code = 404, message = "The function does not exist"),


please revert the indent change here. because @apiresponse is not a separate annotation, but it is part of @ApiResponses

sijie · 2018-12-20T03:14:40Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SinkBase.java

but this is a breaking change no? How can old pulsar admin clients talk to new broker?

sijie · 2018-12-20T03:19:21Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

+            );
+        } catch (RestException re){
+            assertEquals(re.getResponse().getStatusInfo(), Response.Status.BAD_REQUEST);
+            throw re;


keep all assertions in one place. you already catch the exception here, you should also validate @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "Tenant is not provided") rather than rethrowing the exception and use a different way to do assertions.

sijie · 2018-12-20T03:19:32Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

-            topicsToSerDeClassName,
-            className,
-            parallelism,
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "Namespace is not provided")


keep all assertions in one place.

sijie · 2018-12-20T03:19:38Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

-            topicsToSerDeClassName,
-            className,
-            parallelism,
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "Sink Name is not provided")


keep all assertions in one place.

sijie · 2018-12-20T03:20:11Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

-    @Test
-    public void testRegisterSinkZeroParallelism() throws IOException {
-        testRegisterSinkMissingArguments(
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "Sink parallelism should positive number")


keep all assertions in one place.

sijie · 2018-12-20T03:20:16Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

-    @Test
-    public void testRegisterSinkHttpUrl() throws IOException {
-        testRegisterSinkMissingArguments(
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "Invalid Sink Jar")


keep all assertions in one place.

sijie · 2018-12-20T03:20:25Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

    }

-    @Test
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "Sink test-sink already exists")


keep all assertions in one place.

sijie · 2018-12-20T03:20:30Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

    }

-    @Test
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "upload failure")


keep all assertions in one place.

sijie · 2018-12-20T03:20:35Z

...rker/src/test/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2ResourceTest.java

    }

-    @Test
+    @Test(expectedExceptions = RestException.class, expectedExceptionsMessageRegExp = "sink failed to register")


keep all assertions in one place.

srkukarni · 2018-12-21T00:35:26Z

@sijie I'm not sure if this pr breaks backwards compatibility. Its not changing any return types.

sijie · 2018-12-21T00:40:41Z

@srkukarni I would prefer some forms of tests to ensure it doesn't really break BC, because breaking BC is a really bad behavior to users.

sijie

@srkukarni @jerrypeng @aahmed-se I have commented a few examples that return types are changes. Are you guys sure those doesn't break BC?

sijie · 2018-12-21T00:42:30Z

...s/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2Resource.java

    @Path("/{tenant}/{namespace}")
-    public Response listSink(final @PathParam("tenant") String tenant,
-                             final @PathParam("namespace") String namespace) {
+    public List<String> listSink(final @PathParam("tenant") String tenant,


this is returning List<String> instead of Response. doesn't it break the BC?

sijie · 2018-12-21T00:42:54Z

...s/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/v2/SinkApiV2Resource.java

-    public Response getSinkInfo(final @PathParam("tenant") String tenant,
-                                final @PathParam("namespace") String namespace,
-                                final @PathParam("sinkName") String sinkName)
+    public SinkConfig getSinkInfo(final @PathParam("tenant") String tenant,


This is changing from Response to SinkConfig, doesn't it break the BC?

sijie · 2018-12-21T00:43:27Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/SourceBase.java

-                                  final @PathParam("sourceName") String sourceName) throws IOException {
-        return source.getFunctionInfo(
-            tenant, namespace, sourceName);
+    public SourceConfig getSourceInfo(final @PathParam("tenant") String tenant,


This changes Response to SourceConfig, doesn't it break BC?

jerrypeng · 2018-12-21T00:53:00Z

@sijie I think we have already broken backwards compatibility with getFunctionInfo, getStatus, getStats endpoints in previous PRs. To maintain backwards compatibility, I would suggest 1) merge this PR, 2) go back and move all the changes for those endpoints to V3, 3) add the old endpoints back to V2. Add tests to make sure old clients can still talk with V2 endpoints.

aahmed-se · 2018-12-21T00:59:43Z

I will make the changes for the assert statements not sure if we want to invest in BC tests at this time.

sijie · 2018-12-21T01:00:28Z

getFunctionInfo, getStatus, getStats endpoints

can you gusy come up a list of endpoints that are added in 2.3.0 and those were changed in 2.3.0, before we make any decisions?

If they are new endpoints added in 2.3.0, they are okay.
But if they are old endpoints in 2.2.0 and changed in 2.3.0, we should revert those changes and add new endpoints.

aahmed-se · 2018-12-21T02:08:51Z

There is low chance on BC breaking , I am directly returning the object to be serialized by jersey rather than doing it explicitly.

jerrypeng · 2018-12-21T02:14:48Z

@sijie The PR actually doesn’t change the output of any of the endpoints, its just returning the actual object to be serialized by jersey to json instead of doing that explicitly ourselves. This PR will also make it more clear what is actually returned so it will be easier not to break backwards compatibility since currently many endpoints are just returning a Response object which is unclear what is contained within.

However, after this PR lets make sure that no changes have being introduced in the admin api so that 2.3 would break backwards compatibility for 2.2

srkukarni requested review from jerrypeng, rdhabalia, sijie and srkukarni December 12, 2018 01:03

srkukarni assigned aahmed-se Dec 12, 2018

srkukarni added the area/function label Dec 12, 2018

srkukarni added this to the 2.3.0 milestone Dec 12, 2018

sijie requested changes Dec 12, 2018

View reviewed changes

aahmed-se force-pushed the refactorApiCommands branch from efcc2da to dd4e12a Compare December 13, 2018 20:26

aahmed-se changed the title ~~Refactor api commands (WIP)~~ Refactor api commands Dec 13, 2018