[yugabyte] Yugabyte compatible implementation #1143

barroco · 2025-01-02T15:56:57Z

This PR introduces the ability to run core-service with a yugabyte datastore.

Key changes:

ybgin indexes on cells have been removed since they don't support to be scanned with other indexes (which is always the case).
SQL query syntax has been changed to be supported by CRDB as well as Yugabyte. No behaviour change is expected.

Note that it was identified that the current implementation may show some failing test runs of the prober with Yugabyte. Current assumption is that the problem is due to default transaction isolation which is not the same in CRDB (Serializable) and Yugabyte (Snapshot). The later may lead to serialization anomalies for single statement queries. Adapting the code to support this specific use case with Yugabyte may require extensive refactoring which may better be addressed in a separate PR.
Prober tests are catching this issue unreliably. It has been kept disabled for Yugabyte until the implementation is stable.

Commits have been organised in order to facilitate the review.

mickmis

Only minor comments, LGTM otherwise.

mickmis · 2025-01-30T14:22:37Z

build/dev/startup/core_service.sh

-  dlv --headless --listen=:4000 --api-version=2 --accept-multiclient exec --continue /usr/bin/core-service -- \
-  -cockroach_host local-dss-crdb \
+  # Linter is disabled to properly unwrap $DATASTORE_CONNECTION.
+  # shellcheck disable=SC2086


nit: Use an array?
See https://www.shellcheck.net/wiki/SC2086#:~:text=Just%20quoting%20this%20doesn%27t%20work.%20Instead%2C%20you%20should%20have%20used%20an%20array%20(bash%2C%20ksh%2C%20zsh)%3A

mickmis · 2025-01-30T14:29:32Z

build/dev/docker-compose_dss.yaml

@@ -56,7 +61,7 @@ services:
    image: interuss-local/dss
    entrypoint: /usr/bin/db-manager migrate --schemas_dir=/db-schemas/yugabyte/scd --db_version "latest" --cockroach_host local-dss-ybdb --cockroach_user yugabyte --cockroach_port 5433
    depends_on:
-      local-dss-crdb:
+      local-dss-ybdb:


That was an issue present before already, right?
(nothing to change)

mickmis · 2025-01-30T15:05:58Z

build/dev/docker-compose_dss.yaml

    networks:
      - dss_sandbox_default_network
    healthcheck:
      test: wget -O - 'http://localhost/healthy' || exit 1
      interval: 3m
      start_period: 30s
      start_interval: 5s
+    profiles: ["", "with-yugabyte"]


Why the empty string? Is this supposed to represent 'no profile'?

mickmis · 2025-01-30T15:09:37Z

pkg/rid/store/cockroach/subscriptions.go

@@ -87,7 +87,7 @@ func (r *repo) MaxSubscriptionCountInCellsByOwner(ctx context.Context, cells s2.
 	// strict we could keep this count in memory, (or in some other storage).
 	var query = `


q1 and q2 are added but never used. Are they really necessary for yagabyte?

mickmis · 2025-01-30T15:19:02Z

pkg/scd/store/cockroach/constraints.go

-			%s`, constraintFieldsWithoutPrefix, constraintFieldsWithPrefix)
+			%s`,
+			constraintFieldsWithoutPrefix,
+			constraintFieldsWithIndices[0],


If those are referenced individually, might as well directly set them in the query, this would be more readable.

mickmis · 2025-01-30T15:20:52Z

pkg/scd/store/cockroach/constraints.go

@@ -203,7 +228,8 @@ func (c *repo) SearchConstraints(ctx context.Context, v4d *dssmodels.Volume4D) (
 				COALESCE(starts_at <= $3, true)
 			AND
 				COALESCE(ends_at >= $2, true)
-			LIMIT $4`, constraintFieldsWithoutPrefix)
+			LIMIT $4;
+			`, constraintFieldsWithoutPrefix)


Is setting a final ; required for yugabyte? If so, why is it not the case of query on scd_constraints and availabilities above? And if not, why adding it here?

mickmis · 2025-01-30T15:21:51Z

pkg/scd/store/cockroach/operational_intents.go

 				scd_operations
 				(%s)
 			VALUES
 				($1, $2, $3, $4, $5, $6, $7, $8, $9, transaction_timestamp(), $10, $11, $12, $13)
-			RETURNING
-				%s`, operationFieldsWithoutPrefix, operationFieldsWithPrefix)
+			ON CONFLICT (%s) DO UPDATE


Same as above: better specify directly the field names if they are referenced individually.

mickmis · 2025-01-30T15:22:20Z

pkg/scd/store/cockroach/subscriptions.go

 		RETURNING
-			%s`, subscriptionFieldsWithoutPrefix, subscriptionFieldsWithPrefix)
+			%s`,
+			subscriptionFieldsWithoutPrefix,


Same as above: better specify directly the field names if they are referenced individually.

mickmis · 2025-01-30T15:23:19Z

pkg/scd/store/cockroach/subscriptions.go

+				%s = $3::int,
+				%s = $4,
+				%s = $5::int,
+				%s = $6,
+				%s = $7,
+				%s = $8::bool,


Is setting the types here required for yugabyte? If so, why is it not required as well for operational intents?

mickmis · 2025-01-30T15:25:04Z

build/db_schemas/yugabyte/rid/downfrom-v1.0.1-drop-ybgin-indexes.sql

@@ -0,0 +1,2 @@
+CREATE INDEX s_cell_idx ON subscriptions USING ybgin (cells);


If yugabyte is not deployed anywhere, might as well update v1.0.0 IMO. And it probably makes sense keeping doing so until the v1.0.0 is actually a true working v1 that is released.

barroco closed this Jan 2, 2025

barroco reopened this Jan 2, 2025

barroco force-pushed the yugabyte-implementation branch 5 times, most recently from 968bb04 to ea1debf Compare January 7, 2025 18:02

barroco force-pushed the yugabyte-implementation branch from fe834e0 to e735f9a Compare January 14, 2025 14:56

barroco force-pushed the yugabyte-implementation branch 2 times, most recently from f07f19c to 38dd892 Compare January 21, 2025 17:00

Select datastore based on COMPOSE_PROFILE

c93aaf7

barroco force-pushed the yugabyte-implementation branch from 4ddbfb2 to 29fc46a Compare January 28, 2025 15:24

barroco added 6 commits January 28, 2025 16:37

Add healthcheck to ybdb and ensure proper start sequence

cb8946e

RID and SCD initialization

979d44f

Convert to compatible sql statements

bc38a44

Remove ybgin indexes

e137dc1

Use Serializable isolation for transactions

04b398e

Increase ysql_output_buffer_size

68f0328

barroco force-pushed the yugabyte-implementation branch from 29fc46a to 6e5a1d0 Compare January 28, 2025 15:39

Enable qualifier for yugabyte in CI

ed79acc

barroco force-pushed the yugabyte-implementation branch from 6e5a1d0 to ed79acc Compare January 28, 2025 15:41

barroco marked this pull request as ready for review January 28, 2025 15:56

barroco requested review from BenjaminPelletier and mickmis January 28, 2025 17:17

mickmis reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[yugabyte] Yugabyte compatible implementation #1143

[yugabyte] Yugabyte compatible implementation #1143

barroco commented Jan 2, 2025 •

edited

Loading

mickmis left a comment

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

mickmis Jan 30, 2025

		@@ -87,7 +87,7 @@ func (r *repo) MaxSubscriptionCountInCellsByOwner(ctx context.Context, cells s2.
		// strict we could keep this count in memory, (or in some other storage).
		var query = `

		@@ -0,0 +1,2 @@
		CREATE INDEX s_cell_idx ON subscriptions USING ybgin (cells);

[yugabyte] Yugabyte compatible implementation #1143

Are you sure you want to change the base?

[yugabyte] Yugabyte compatible implementation #1143

Conversation

barroco commented Jan 2, 2025 • edited Loading

mickmis left a 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

barroco commented Jan 2, 2025 •

edited

Loading