External service #1074

snkas · 2023-11-28T15:22:45Z

External service concept is added. In order to have small PRs rather than large, this is the first with basic functionality for external services.

This PR encompasses:

Database table: service
Basic database operations for external services (list, create, get, update, delete)
API endpoints for basic operations on external services
Corresponding Python API generated
Database proptests
API integration tests

Is this a user-visible change (yes/no): yes

lalithsuresh

Great start! Could you also integrate ExternalService into the src/db/test.rs?

lalithsuresh · 2023-11-29T03:43:55Z

crates/pipeline-types/src/config.rs

+
+/// An external service's configuration.
+#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize, ToSchema)]
+pub enum ExternalServiceConfig {


If the type is already called ExternalService, you don't need to also repeat the prefix in the inner *Config types.

~~Inlined them into the Service enumeration~~ OpenAPI Python did not like that, instead using explicit types MysqlConfig and PostgresConfig

lalithsuresh · 2023-11-29T03:44:41Z

crates/pipeline_manager/migrations/V6__add_external_service.sql

+CREATE TABLE IF NOT EXISTS external_service (
+    id uuid PRIMARY KEY,                  -- Unique identifier (used primarily for foreign key relations)
+    tenant_id uuid NOT NULL,              -- Tenant the external service belongs to
+    name varchar NOT NULL,                -- Immutable unique name given by the tenant (e.g., mysql-example) used to refer to it in the API


Is it already immutable in our API?

Yes, the PATCH /external_services/id endpoint only allows updating description and config.

We could also enforce it in the database with a trigger potentially (?)

crates/pipeline_manager/src/api.rs

crates/pipeline_manager/src/db/mod.rs

lalithsuresh · 2023-11-29T03:58:13Z

Bikeshed

@ryzhyk @gz regarding the API entity's name, we have ExternalService for now. Any other suggestions?
@snkas and I think it's precise, but it's kind of long.

ryzhyk · 2023-11-29T04:04:30Z

ExternalServide seems adequate. The External prefix could be dropped without introducing ambiguity.

lalithsuresh · 2023-11-29T04:44:26Z

I'd be okay changing it to Service. A bit overloaded, but clean.

snkas · 2023-11-29T13:18:13Z

Changing name to Service

snkas · 2023-11-29T16:15:02Z

I've rebased with main, incorporated feedback (see comments) and implemented the test database.

Which services do we add at the start? I've temporarily added MysqlConfig and PostgresConfig as the two services currently implemented. In the future these current services might change (remove user/password for instance).
I'm planning to implement a few API tests, possibly in python/test.py or somewhere similar; EDIT: and as well in the integration tests
Should I also add a custom database test? Beyond the automated proptests currently done.

After this PR it might be also time to make MaybeSecretRef an API object? As it will likely be used in the services.

lalithsuresh · 2023-11-29T17:11:50Z

I've rebased with main, incorporated feedback (see comments) and implemented the test database.

* Which services do we add at the start? I've temporarily added MysqlConfig and PostgresConfig as the two services currently implemented. In the future these current services might change (remove user/password for instance).

It's more important to start with Kafka (let's start from all the services we use for connectors). So MySQL and Kafka would be the two important ones to start with. I don't think we need Postgres yet.

* I'm planning to implement a few API tests, possibly in `python/test.py` or somewhere similar; EDIT: and as well in the integration tests

It's probably better to add external tests like python API tests after you integrate with connectors. As such, there's no external workflow we can support with this PR that depends on services being configured correctly.

* Should I also add a custom database test? Beyond the automated proptests currently done.

The proptests should do.

After this PR it might be also time to make MaybeSecretRef an API object? As it will likely be used in the services.

Let's get the connector integration working first.

lalithsuresh · 2023-11-29T17:13:11Z

crates/pipeline_manager/src/db/mod.rs

+        if modified_rows == 0 {
+            Err(DBError::UnknownService { service_id })
+        } else {
+            assert_eq!(modified_rows, 1);


Log this as an error. Don't assert outside tests.

Changed to:

if modified_rows != 1 { error!("More than one row ({modified_rows}) was modified when updating service {service_id}"); }

Why not return an error in this case? This looks like it should not happen.

Changed to:

if modified_rows > 0 { Ok(()) } else { Err(DBError::UnknownService { service_id }) }

snkas · 2023-11-30T12:51:16Z

Rebased with main, incorporated feedback (removed assert and replaced Postgres service with Kafka service), added basic service integration test in integration_test.rs

lalithsuresh · 2023-11-30T16:24:43Z

crates/pipeline-types/src/config.rs

+
+/// A service's configuration.
+#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize, ToSchema)]
+#[serde(rename_all = "snake_case")]


Why is the snake_case required?

Added:
// snake_case such that the enumeration variants are not capitalized in (de-)serialization

Adds the Service type with basic functionality to the pipeline-manager. This encompasses: - Database table: service - Basic database operations (list, create, get, update, delete) - API endpoints - Corresponding generated Python API - Database proptests - API integration test Signed-off-by: Simon Kassing <simon.kassing@feldera.com>

snkas requested a review from lalithsuresh November 28, 2023 15:22

lalithsuresh requested changes Nov 29, 2023

View reviewed changes

snkas force-pushed the external-service branch from 3fdb010 to c01aa1e Compare November 29, 2023 14:10

lalithsuresh reviewed Nov 29, 2023

View reviewed changes

snkas force-pushed the external-service branch 2 times, most recently from 9e74c9b to 01067a1 Compare November 30, 2023 12:47

snkas requested a review from lalithsuresh November 30, 2023 13:10

lalithsuresh reviewed Nov 30, 2023

View reviewed changes

snkas force-pushed the external-service branch from 462c86f to c92be4a Compare November 30, 2023 18:19

lalithsuresh approved these changes Nov 30, 2023

View reviewed changes

snkas force-pushed the external-service branch from c92be4a to ca23685 Compare November 30, 2023 19:03

snkas merged commit e8f4fd1 into main Nov 30, 2023
5 checks passed

snkas deleted the external-service branch November 30, 2023 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

External service #1074

External service #1074

snkas commented Nov 28, 2023 •

edited

lalithsuresh left a comment •

edited

lalithsuresh Nov 29, 2023

snkas Nov 29, 2023 •

edited

lalithsuresh Nov 29, 2023

snkas Nov 29, 2023

snkas Nov 29, 2023

lalithsuresh commented Nov 29, 2023 •

edited

ryzhyk commented Nov 29, 2023

lalithsuresh commented Nov 29, 2023

snkas commented Nov 29, 2023

snkas commented Nov 29, 2023 •

edited

lalithsuresh commented Nov 29, 2023

lalithsuresh Nov 29, 2023

snkas Nov 30, 2023

lalithsuresh Nov 30, 2023 •

edited

snkas Nov 30, 2023

snkas commented Nov 30, 2023 •

edited

lalithsuresh Nov 30, 2023

snkas Nov 30, 2023

External service #1074

External service #1074

Conversation

snkas commented Nov 28, 2023 • edited

lalithsuresh left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snkas Nov 29, 2023 • edited

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

lalithsuresh commented Nov 29, 2023 • edited

Bikeshed

ryzhyk commented Nov 29, 2023

lalithsuresh commented Nov 29, 2023

snkas commented Nov 29, 2023

snkas commented Nov 29, 2023 • edited

lalithsuresh commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalithsuresh Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snkas commented Nov 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snkas commented Nov 28, 2023 •

edited

lalithsuresh left a comment •

edited

snkas Nov 29, 2023 •

edited

lalithsuresh commented Nov 29, 2023 •

edited

snkas commented Nov 29, 2023 •

edited

lalithsuresh Nov 30, 2023 •

edited

snkas commented Nov 30, 2023 •

edited