REST endpoints #6

sknot-rh · 2020-12-03T15:00:18Z

Adding functionality to listTopics, describe particular topic, create topic, update, and delete topic.

Full docs can be found here

For now quay registry set to sknot.
It is possible to deploy app by posting docker/deployment.yaml file to k8s server.

that is extensible through the Java SPI. Signed-off-by: Graeme McRobert <[email protected]>

Contains an initial implementation of the Strimzi Admin server. The main application is in the `http-server` module. It creates a vertx verticle which loads routes from modules on the classpath using the Java service loader and then opens port 8080 to listen for requests. There are currently 2 modules containing routes. They are the `health` module and the `graphql` module. The `health` module contains a `liveness` and a `status` resource which return a static `status:ok` json response. The `graphql` module implements the graphql server. The graphql server creates a base server with no queries or mutations. Currently, only sample queries are implemented. The queries are defined in business log modules of which the `kafka-admin` module is defined as an example. The graphql server uses the Java service loader as well to register graphql schema and implementation details to the graphql server. If the `VERTXWEB_ENVIRONMENT` envar is set to `dev`, the graphql server will also serve `GraphiQL` as per the `vertx-web-graphql` documentation. To server can be built using `mvn clean package` and can then be run using the following: java -cp kafka-admin/target/kafka-admin-1.0-SNAPSHOT-fat.jar:health/target/health-1.0-SNAPSHOT-fat.jar:grapql/target/graphql-s.0-SNAPSHOT-far.jar:http-server/target/http-server-1.0-SNAPSHOT-fat.jar io.strimzi.admin.Main Tests are to follow. Signed-off-by: Graeme McRobert <[email protected]>

* Added the maven-dependency-plugin `Analyze` rules and tidied the pom files. * Added try/catch blocks for i/o operations in the executeBlocking blocks. * Added Javadoc to main classes. * Removed the duplicated sample data and made the data publicly visible so that both the TopicHandler and TopicListHandler could access it. * Uppercased the name of the sample data as it is a constant Signed-off-by: Graeme McRobert <[email protected]>

Signed-off-by: Stanislav Knot <[email protected]>

wtrocki · 2020-12-03T15:38:11Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

+    #id: ID
+    name: String
+
+    """@transient"""


Suggested change

"""@transient"""

wtrocki · 2020-12-03T15:39:41Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

+extend type Mutation {
+    createTopic(input: CreateTopicInput!): TopicDescription
+    updateTopic(input: MutateTopicInput!): TopicDescription
+    deleteTopic(name: String!): String


Rule of thumb is to never ever return Scalar from the API. It will make our API breaking with each change - no ability to build non breaking API

Oh. Good to know! Thanks.

wtrocki · 2020-12-03T15:43:21Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

 }

-type Topic {
+type TopicOnlyName {


No strong opinion but this might be redundant due to the capabilities of the client to make query with selection set containing only name. Making type for it makes things much more complex and also will put constraints on growing API.

This is not comment to fix it etc. I'm just mentioning it from the point of schema good practices as even if someone will request extra fields in topic query they will get null data.

I agree. This is still a WIP and I was expecting (hoping for) a comment like this.

Totally agree we don't need this.

wtrocki · 2020-12-03T15:45:04Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

 }

-type Topic {
+type TopicOnlyName {
+    #id: ID


In GraphQL ID's are sacred due to client side caching. If we get list of the topics and then operate on specific topics to get extra data client side will usually normalize those etc.

Frameworks like Apollo etc. tend to have extra configuration to handle objects without id's etc. If we apply suggestion from above this would not be issue

wtrocki · 2020-12-03T15:48:18Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

+    config: TopicConfig
+}
+
+type Partitions {


Trivial/Opiniated
There are two class of types in GraphQL;

Embeeded types

Root (Query and Mutation return types)

I tend to organize schema to add extra comment that this type is not used in queries/mutations.

""" embedded """

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

wtrocki · 2020-12-03T15:51:03Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

+
+extend type Query {
+    topicList(filter: String): [TopicOnlyName!]
+    topicDescription(name: String!): TopicDescription!


Trivial/Style

This seems to be leaking server side implementation details.
If I want to have some information on topic I should be able to get it with the fields that I have request it and if backend needs to do 6-7 calls to do so then so be it.
Making client doing multiple calls is more restful approach

So you would rather see topicDescription returning just [isInternal, name, partitions] and another query (for example) topicConfiguration to get all these configs? https://kafka.apache.org/documentation/#topicconfigs

This is coming from GrapHQL ideology that there is query that returns an entire object and it's relationships and the client can have the freedom to pick what he wants.

Splitting schema objects towards what client might want (list of just names or list of the topic or the configs etc.) seems to be more aligned with the way REST works.

Then client can pick query by selecting what they want:
https://i0.wp.com/css-tricks.com/wp-content/uploads/2020/01/Screen-Shot-2020-01-27-at-11.32.20.png?resize=3104%2C1978&ssl=1

Obviously, this is your project and there might be different reasons why we want to split topic into 3 different objects so do not feel that I'm really suggesting something that should be done.
More like trying to help with schema and avoiding some pitfalls early on.

wtrocki · 2020-12-03T15:53:17Z

Amazing work @sknot-rh . Really good work on querying and mutations. My comments are opiniated take on the how GraphQL schema might work. My intention was to share some points based on the schemas I have written in the past. Do not feel that you need to apply all of suggestion etc.

Signed-off-by: Stanislav Knot <[email protected]>

wtrocki · 2020-12-03T16:28:44Z

I'm curious - do we have any ability to generate id for the topics?

sknot-rh · 2020-12-03T16:35:08Z

Currently, topic identity is determined from its name. There is a KIP for adding a topic ID (@tombentley knows more). I think as an ID we can use for example (namespaceName?+separator+)clusterName+separator+topicName. Do you have any suggestions?

wtrocki · 2020-12-03T16:54:01Z

It will be good to actually have ID for the topics. If that id will be identified by name.
Sounds like to early optimalization and it will require circling thru arrays to build it but it might be worth to create issue for it in the future. Not having ID will not cause any major problems unless we want to use client side cache - which will make no sense for project like this.

ppatierno · 2020-12-03T16:41:55Z

docker/deployment.yaml

+            - name: KAFKA_ADMIN_BOOTSTRAP_SERVERS
+              value: "my-cluster-kafka-bootstrap:9092"
+            - name: VERTXWEB_ENVIRONMENT
+              value: "dev"


it should be not here for a production-ready environment, it should be removed to be the default one.

ppatierno · 2020-12-03T16:45:46Z

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/KafkaAdminService.java

-                )
-                .build();
+                AdminClientProvider acp = new AdminClientProvider(vertx, config);
+                acp.open();


if the Kafka cluster is not ready, it's running on the Vert.x main thread blocking it. We should avoid that doing asynchronously.

ppatierno · 2020-12-03T16:46:21Z

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/KafkaAdminService.java

-                .build();
+                AdminClientProvider acp = new AdminClientProvider(vertx, config);
+                acp.open();
+                // todo close acp


what about this todo? are working on it? why should we close the acp here?

I am not sure whether we need to close AdminClient at some point.

ppatierno · 2020-12-03T16:48:06Z

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/handlers/TopicCreateHandler.java

+                createOrMutateTopicConfigInput.setReplicationFactor(Long.parseLong(val));
+                newTopic.setReplicationFactor(Short.parseShort(val));
+            }
+            if (configObject.get("pairs") != null) {


is this the topic configuration options, why not calling "config" instead of "pairs"?

The query would look like this (nested config). I am not sure we like that.

query { topicDescription(name: "topicX") { name config { partitionCount replicationFactor config { key value } } } }

ppatierno · 2020-12-03T16:52:17Z

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql

 }

-type Topic {
+type TopicOnlyName {


Totally agree we don't need this.

Signed-off-by: Stanislav Knot <[email protected]>

tombentley · 2020-12-04T09:01:05Z

Currently, topic identity is determined from its name. There is a KIP for adding a topic ID (@tombentley knows more). I think as an ID we can use for example (namespaceName?+separator+)clusterName+separator+topicName. Do you have any suggestions?

https://cwiki.apache.org/confluence/display/KAFKA/KIP-516%3A+Topic+Identifiers it will help in avoiding the current ambiguity when a topic is deleted and a new topic with the same name is created. It's worth understanding the proposed changes for the AdminClient, and also the future work envisaged.

Signed-off-by: Stanislav Knot <[email protected]>

docker/deployment.yaml

Signed-off-by: Stanislav Knot <[email protected]>

graphql/src/main/java/io/strimzi/admin/graphql/GraphQLService.java

gmcrobert · 2020-12-11T09:17:44Z

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/AdminClientWrapper.java

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


So I am not sure what the value of this wrapper is. It seems to just duplicate functionality in the KafkaAdminClient which itself is a wrapper for the Kafka AdminClient. If you are having the wrapper, I would have thought it would have been more useful turning the async code into promise based futures so the methods return a Vertx Future to allow you to use the .onSuccess, .onFailure, .onComplete methods on the Future. Vertx 4 does this in the Vertx KafkaClient but it is not GA yet. From experience, I am also nervous throwing exceptions in a Vertx environment because Vertx has a tendency to swallow exceptions and it is not obvious where things end up. It is worse than a goto in my view.

gmcrobert · 2020-12-11T09:34:47Z

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/handlers/CommonHandler.java

+
+import java.util.Map;
+
+public class CommonHandler {


Just an observation, I would have dealt with the security differently. The routingContext is provided to keep a context for the request and be available from any handlers. We are also making the routingContext available in any DataFetcher. I would have created security handlers that deal with the different authentication mechanisms as we are going to support both SASL and OAuth2 and put them at the top of the handler list so that they get called first in the pipeline. The security handlers can create a "User" object on the routingContext allowing any other handler or DataFetcher to access it and use it to decorate the properties.

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/handlers/CommonHandler.java

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/handlers/TopicCreateHandler.java

Signed-off-by: Stanislav Knot <[email protected]>

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/handlers/TopicListHandler.java

Signed-off-by: Stanislav Knot <[email protected]>

gmcrobert and others added 7 commits December 3, 2020 15:49

Prototype of a vertx HTTP server for strimzi-admin

f3a1cc1

that is extensible through the Java SPI. Signed-off-by: Graeme McRobert <[email protected]>

init3

26780d7

Signed-off-by: Stanislav Knot <[email protected]>

conteinerize

c929e6f

Signed-off-by: Stanislav Knot <[email protected]>

fixes

f5073be

Signed-off-by: Stanislav Knot <[email protected]>

topic config

a266ab7

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh requested a review from ppatierno December 3, 2020 15:00

sknot-rh marked this pull request as draft December 3, 2020 15:00

Adding pain. Ehm, I mean checkstyle + spotbugs

c7e1900

Signed-off-by: Stanislav Knot <[email protected]>

wtrocki reviewed Dec 3, 2020

View reviewed changes

kafka-admin/src/main/resources/graphql-schema/kafka-admin.graphql Outdated Show resolved Hide resolved

wtrocki reviewed Dec 3, 2020

View reviewed changes

support creating topic with configuration (key:value)s

34fda8b

Signed-off-by: Stanislav Knot <[email protected]>

ppatierno reviewed Dec 3, 2020

View reviewed changes

Json scalar type

2682f65

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh added 2 commits December 4, 2020 10:15

do not use scalars

fc27501

Signed-off-by: Stanislav Knot <[email protected]>

service

1e27e9a

Signed-off-by: Stanislav Knot <[email protected]>

ppatierno reviewed Dec 4, 2020

View reviewed changes

docker/deployment.yaml Outdated Show resolved Hide resolved

improved schema

0c5c183

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh force-pushed the graphql-work branch from 088fe5d to 0c5c183 Compare December 4, 2020 14:33

gmcrobert reviewed Dec 11, 2020

View reviewed changes

full listing

948d6b9

Signed-off-by: Stanislav Knot <[email protected]>

k-wall reviewed Dec 11, 2020

View reviewed changes

kafka-admin/src/main/java/io/strimzi/admin/kafka/admin/handlers/TopicListHandler.java Outdated Show resolved Hide resolved

maybe

6d7c5b2

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh force-pushed the graphql-work branch from eadc177 to 6d7c5b2 Compare December 11, 2020 17:18

sknot-rh added 9 commits December 14, 2020 17:06

fix expired token exception looping

860a22d

Signed-off-by: Stanislav Knot <[email protected]>

refactor

0a6d7eb

Signed-off-by: Stanislav Knot <[email protected]>

refactor

666d38d

Signed-off-by: Stanislav Knot <[email protected]>

update the topic

435ae88

Signed-off-by: Stanislav Knot <[email protected]>

more refactoring

f7b8235

Signed-off-by: Stanislav Knot <[email protected]>

comments

509a55b

Signed-off-by: Stanislav Knot <[email protected]>

rest

d09599c

Signed-off-by: Stanislav Knot <[email protected]>

refactor

02f942e

Signed-off-by: Stanislav Knot <[email protected]>

remove wrapper

3469de3

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh changed the title ~~Graphql work~~ Graphql and REST Jan 19, 2021

craicoverflow mentioned this pull request Jan 21, 2021

Use data plane REST API for topic operations redhat-developer/app-services-cli#162

Closed

sknot-rh added 2 commits January 28, 2021 12:30

refactor + new API

e45b741

Signed-off-by: Stanislav Knot <[email protected]>

some codes

9065c63

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh force-pushed the graphql-work branch from fc53741 to 9065c63 Compare January 28, 2021 12:40

sknot-rh added 5 commits January 28, 2021 16:28

refactor

f89fdff

Signed-off-by: Stanislav Knot <[email protected]>

lol, removing graphQL

624be7f

Signed-off-by: Stanislav Knot <[email protected]>

refactor

2b189e3

Signed-off-by: Stanislav Knot <[email protected]>

offset and limit

ba35198

Signed-off-by: Stanislav Knot <[email protected]>

return codes

128fe53

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh changed the title ~~Graphql and REST~~ REST endpoints Feb 1, 2021

sknot-rh added 2 commits February 3, 2021 08:53

create topic uses nested settings object

0ff6a41

Signed-off-by: Stanislav Knot <[email protected]>

metrics

13e88b9

Signed-off-by: Stanislav Knot <[email protected]>

sknot-rh requested a review from ppatierno February 11, 2021 14:34

consumer groups

74029d5

Signed-off-by: Stanislav Knot <[email protected]>

Base automatically changed from master to main March 25, 2021 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST endpoints #6

REST endpoints #6

sknot-rh commented Dec 3, 2020 •

edited

Loading

wtrocki Dec 3, 2020

wtrocki Dec 3, 2020

sknot-rh Dec 3, 2020

wtrocki Dec 3, 2020

sknot-rh Dec 3, 2020

ppatierno Dec 3, 2020

wtrocki Dec 3, 2020

wtrocki Dec 3, 2020

wtrocki Dec 3, 2020

sknot-rh Dec 3, 2020

wtrocki Dec 3, 2020 •

edited

Loading

wtrocki commented Dec 3, 2020

wtrocki commented Dec 3, 2020

sknot-rh commented Dec 3, 2020 •

edited

Loading

wtrocki commented Dec 3, 2020 •

edited

Loading

ppatierno Dec 3, 2020

sknot-rh Dec 4, 2020

ppatierno Dec 3, 2020

ppatierno Dec 3, 2020

sknot-rh Dec 4, 2020

ppatierno Dec 3, 2020

sknot-rh Dec 4, 2020

ppatierno Dec 3, 2020

tombentley commented Dec 4, 2020

gmcrobert Dec 11, 2020

gmcrobert Dec 11, 2020

REST endpoints #6

Are you sure you want to change the base?

REST endpoints #6

Conversation

sknot-rh commented Dec 3, 2020 • 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

wtrocki Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

wtrocki commented Dec 3, 2020

wtrocki commented Dec 3, 2020

sknot-rh commented Dec 3, 2020 • edited Loading

wtrocki commented Dec 3, 2020 • 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

tombentley commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sknot-rh commented Dec 3, 2020 •

edited

Loading

wtrocki Dec 3, 2020 •

edited

Loading

sknot-rh commented Dec 3, 2020 •

edited

Loading

wtrocki commented Dec 3, 2020 •

edited

Loading