Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data Transform Admin APIs #13293

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Sep 6, 2023

Stub out the initial Admin HTTP endpoints for Data Transforms.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

  • none

@rockwotj rockwotj force-pushed the rockwood/admin-api-stubs branch 2 times, most recently from 4a0acfa to 66d614b Compare September 6, 2023 18:29
@rockwotj rockwotj marked this pull request as ready for review September 7, 2023 01:00
@rockwotj
Copy link
Contributor Author

rockwotj commented Sep 7, 2023

CI Failures: #12659, #13301

@rystsov
Copy link
Contributor

rystsov commented Sep 7, 2023

CI Failures: #12659, #13301

@rockwotj I doubt that it's the case but #13301 is a new failure which potentially could be introduced by this PR; please emphasize it to make it obvious for the reviewers e.g. like there

@rockwotj
Copy link
Contributor Author

rockwotj commented Sep 7, 2023

Ah I had put it in the issue, but will duplicate that info here as well. Thanks for the reminder Denis!

#13301 (comment)

Copy link
Contributor

@NyaliaLui NyaliaLui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice stuff, I like the idea of a boilerplate PR for new Admin APIs.

@rockwotj rockwotj force-pushed the rockwood/admin-api-stubs branch from 66d614b to 1d28390 Compare September 11, 2023 17:33
@rockwotj rockwotj requested a review from NyaliaLui September 11, 2023 17:34
@rockwotj rockwotj force-pushed the rockwood/admin-api-stubs branch from 1d28390 to 5a26ad0 Compare September 12, 2023 13:49
NyaliaLui
NyaliaLui previously approved these changes Sep 12, 2023
Copy link
Contributor

@NyaliaLui NyaliaLui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me as long as the CI failures are unrelated

@@ -0,0 +1,163 @@
{
"apiVersion": "0.0.1",
"swaggerVersion": "1.2",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The swagger on the admin API doesn't really work and isn't consistent, but I think we would generally have better luck with tooling when we do get to fixing it, if it was 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry is the suggestion here to make this one use the 2.0 format?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the swagger for the admin API is a huge project, I think it would be less burdensome if we use v2.0 for new stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I will update the transform doc to be v2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seastar does not actually support the v2 format even if seastar2code.py says that it does. Here's the patch, but the code generator explodes. @BenPope do you think we should fix it now? To be honest I would love a better format than swagger for these APIs anyways.

diff --git a/src/v/redpanda/admin/api-doc/transform.json b/src/v/redpanda/admin/api-doc/transform.json
index 7b2c3b04f..0b50ff949 100644
--- a/src/v/redpanda/admin/api-doc/transform.json
+++ b/src/v/redpanda/admin/api-doc/transform.json
@@ -1,64 +1,89 @@
 {
-  "apiVersion": "0.0.1",
-  "swaggerVersion": "1.2",
-  "basePath": "/v1",
-  "resourcePath": "/transform",
-  "apis": [
-    {
-      "path": "/v1/transform/deploy",
-      "operations": [
-        {
-          "method": "POST",
-          "summary": "Deploy a transform based on the provided metadata and wasm binary in the request payload. The payload should be the transform metadata in JSON format immediately followed by the WebAssembly binary without any delimiters.",
-          "nickname": "deploy_transform",
-          "consumes": [
-            "application/json+wasm"
-          ],
-          "type": "void",
-          "produces": [
-            "application/json"
-          ]
+  "swagger": "2.0",
+  "info": {
+    "title": "Data Transforms API",
+    "description": "API for WebAssembly powered Data Transforms in Redpanda",
+    "version": "0.0.1"
+  },
+  "basePath": "/v1/transform",
+  "schemes": [
+    "http"
+  ],
+  "consumes": [],
+  "produces": [
+    "application/json"
+  ],
+  "paths": {
+    "/deploy": {
+      "post": {
+        "operationId": "deploy_transform",
+        "description": "Deploy a transform based on the provided metadata and wasm binary in the request payload. The payload should be the transform metadata in JSON format immediately followed by the WebAssembly binary without any delimiters.",
+        "consumes": [
+          "application/json+wasm"
+        ],
+        "parameters": [
+          {
+            "name": "request_metadata",
+            "in": "body",
+            "description": "the metadata for the following wasm binary",
+            "required": true,
+            "schema": {
+              "$ref": "#/definitions/deploy_transform_request"
+            }
+          }
+        ],
+        "responses": {
+          "200": {
+            "type": "void"
+          }
         }
-      ]
+      }
     },
-    {
-      "path": "/v1/transform/delete",
-      "operations": [
-        {
-          "method": "POST",
-          "summary": "Delete a transform based on the provided name in the request payload.",
-          "type": "void",
-          "nickname": "delete_transform",
-          "consumes": [
-            "application/json"
-          ],
-          "produces": [
-            "application/json"
-          ]
+    "/list": {
+      "get": {
+        "operationId": "list_transform",
+        "description": "Delete a transform based on the provided name in the request payload.",
+        "consumes": [
+          "application/json"
+        ],
+        "parameters": [],
+        "responses": {
+          "200": {
+            "type": "array",
+            "items": {
+              "$ref": "#/definitions/transform_metadata"
+            }
+          }
         }
-      ]
+      }
     },
-    {
-      "path": "/v1/transform/list",
-      "operations": [
-        {
-          "method": "GET",
-          "type": "array",
-          "items": {
-            "type": "transform_metadata"
-          },
-          "summary": "List all transforms, the status report contains a cluster wide aggregated view of the live transform execution state.",
-          "nickname": "list_transforms",
-          "produces": [
-            "application/json"
-          ]
+    "/delete": {
+      "post": {
+        "operationId": "delete_transform",
+        "consumes": [
+          "application/json"
+        ],
+        "parameters": [
+          {
+            "name": "request",
+            "in": "body",
+            "description": "the request for the delete operation",
+            "required": true,
+            "schema": {
+              "$ref": "#/definitions/delete_transform_request"
+            }
+          }
+        ],
+        "responses": {
+          "200": {
+            "type": "void"
+          }
         }
-      ]
+      }
     }
-  ],
-  "models": {
+  },
+  "definitions": {
     "deploy_transform_request": {
-      "id": "deploy_transform_request",
       "properties": {
         "name": {
           "type": "string"
@@ -75,14 +100,13 @@
         "environment": {
           "type": "array",
           "items": {
-            "type": "environment_variable"
+            "type": "#/definitions/environment_variable"
           },
           "description": "The environment variable configuration for a transform"
         }
       }
     },
     "delete_transform_request": {
-      "id": "delete_transform_request",
       "properties": {
         "name": {
           "type": "string"
@@ -90,7 +114,6 @@
       }
     },
     "transform_metadata": {
-      "id": "transform_metadata",
       "properties": {
         "name": {
           "type": "string"
@@ -113,14 +136,13 @@
         "environment": {
           "type": "array",
           "items": {
-            "type": "environment_variable"
+            "type": "#/definitions/environment_variable"
           },
           "description": "The environment variable configuration for a transform"
         }
       }
     },
     "partition_transform_status": {
-      "id": "partition_transform_status",
       "description": "The status of a single partition's transform",
       "properties": {
         "node_id": {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenPope do we have some information on how to make new API files? I think people are copy-pasting without knowing of a better way (i myself am clueless). Given the existence of a multiple (2, 3?) different styles, do we gain much from selecting one over the other if the transition ends up being mechanical?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ben and I had a conversation in DM about this (probably should have had this in a core channel) but there are no examples of adding models in the admin endpoints for swagger v2.

Honestly I like v2 much more, but would rather prefer a better codegen mechanism here instead of invalid json files :/

@BenPope
Copy link
Member

BenPope commented Sep 12, 2023

For the docs team, these new endpoints will need adding to this page: https://docs.redpanda.com/api/admin-api/

@rockwotj rockwotj requested a review from BenPope September 12, 2023 18:48
@rockwotj
Copy link
Contributor Author

/ci-repeat

@rockwotj
Copy link
Contributor Author

CI Failure: #13181

dotnwat
dotnwat previously approved these changes Sep 13, 2023
Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
Just do the initial wiring up and boilerplate of registering the routes.
Currently all the routes will throw a 404.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@rockwotj rockwotj dismissed stale reviews from dotnwat and NyaliaLui via 1755f76 September 14, 2023 16:06
@rockwotj rockwotj force-pushed the rockwood/admin-api-stubs branch from 5a26ad0 to 1755f76 Compare September 14, 2023 16:06
@rockwotj
Copy link
Contributor Author

Make the API more REST-ful

@rockwotj
Copy link
Contributor Author

CI Failures: #11371, #11998

@rockwotj rockwotj merged commit 7f512fb into redpanda-data:dev Sep 14, 2023
9 checks passed
@rockwotj rockwotj deleted the rockwood/admin-api-stubs branch September 14, 2023 23:33
@Feediver1
Copy link

@rockwotj Is there anything re Data Transforms API still required for https://docs.redpanda.com/current/labs/data-transform/data-transform-api/?
cc: @micheleRP

@rockwotj
Copy link
Contributor Author

rockwotj commented Oct 6, 2023

There are new Admin APIs we'll want to add here: https://docs.redpanda.com/api/admin-api/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants