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.

Sign up for GitHub

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

Jump to bottom

Add migrations for shards table in postgres metastore #4119

Merged

guilload merged 1 commit into quickwit-oss:main from kamalesh0406:main

Nov 16, 2023

Contributor

kamalesh0406 commented Nov 11, 2023 •

edited

Loading

Description

This is the first set of changes to support the shards API in the postgres metastore(#3810). There are couple of items that still need to be addressed:

Using index_uid vs index_id for the table, the ingestv2_api seems to use (index_id, shard_id) whereas the filebacked index uses index_uid. What is everyone's consensus on the right column for this?
Do we need to store the SourceCheckpoint similar to this https://github.com/quickwit-oss/quickwit/blob/main/quickwit/quickwit-metastore/src/metastore/file_backed_metastore/file_backed_index/shards.rs#L42 ? If so, I think the right approach might be to create another table for it.
We are using an auto increement column type for next_shard_id in this PR. But here https://github.com/quickwit-oss/quickwit/blob/main/quickwit/quickwit-metastore/src/metastore/file_backed_metastore/file_backed_index/shards.rs#L144 we use the next_shard_id from the subrequest. Can the subrequest have a next_shard_id that can deviate from the indexes next_shard_id? If yes, then I would need to remove the auto-increement.

How was this PR tested?

sqlx migrate run --database-url postgres://quickwit-dev:quickwit-dev@localhost:5432/quickwit-metastore-dev --source migrations/postgresql

kamalesh0406 requested review from guilload and fmassot

November 11, 2023 17:58

Member

guilload commented Nov 11, 2023

Let's use index_uid for this table. The index_id -> index_uid resolution happens in the control-plane.
No, we don't need to store the SourceCheckpoint in a similar way. It's just a shortcut that I took to reuse the checkpointing logic that we had, but in reality all we need is a publish_position per shard.
They should not deviate, so auto-increment is the way to go.

Contributor Author

kamalesh0406 commented Nov 11, 2023

Okay that sounds good @guilload . I think my current changes are fine then!

guilload approved these changes

View reviewed changes

quickwit/quickwit-metastore/migrations/postgresql/12_create-shards.up.sql Outdated

+              CREATE TYPE SHARDSTATE AS ENUM ('SHARD_STATE_UNSPECIFIED', 'SHARD_STATE_OPEN', 'SHARD_STATE_UNAVAILABLE', 'SHARD_STATE_CLOSED');
+              CREATE TABLE IF NOT EXISTS shards (
+                  index_uid VARCHAR(288) NOT NULL,

Member

guilload Nov 13, 2023

Why 288 and not 282? (255 + 1 for : separator + 26 for ULID)?

Contributor Author

kamalesh0406 Nov 14, 2023

Yup it should be 282, I mistaked it for 288.

quickwit/quickwit-metastore/migrations/postgresql/12_create-shards.up.sql Outdated

+              CREATE TABLE IF NOT EXISTS shards (
+                  index_uid VARCHAR(288) NOT NULL,
+                  source_id VARCHAR(50) NOT NULL,

Member

guilload Nov 13, 2023

Suggested change

      
                source_id VARCHAR(50) NOT NULL,
          
                source_id VARCHAR(255) NOT NULL,

quickwit/quickwit-metastore/migrations/postgresql/12_create-shards.up.sql Outdated

+                  index_uid VARCHAR(288) NOT NULL,
+                  source_id VARCHAR(50) NOT NULL,
+                  shard_id BIGSERIAL,
+                  leader_id VARCHAR(50) NOT NULL,

Member

guilload Nov 13, 2023

Suggested change

      
                leader_id VARCHAR(50) NOT NULL,
          
                leader_id VARCHAR(255) NOT NULL,

quickwit/quickwit-metastore/migrations/postgresql/12_create-shards.up.sql Outdated

+                  source_id VARCHAR(50) NOT NULL,
+                  shard_id BIGSERIAL,
+                  leader_id VARCHAR(50) NOT NULL,
+                  follower_id VARCHAR(50),

Member

guilload Nov 13, 2023

Suggested change

      
                follower_id VARCHAR(50),
          
                follower_id VARCHAR(255),

quickwit/quickwit-metastore/migrations/postgresql/12_create-shards.up.sql Outdated

+                  follower_id VARCHAR(50),
+                  shard_state SHARDSTATE NOT NULL,
+                  publish_position_inclusive bytea NOT NULL,
+                  publish_token VARCHAR(50),

Member

guilload Nov 13, 2023

Suggested change

      
                publish_token VARCHAR(50),
          
                publish_token VARCHAR(255),

kamalesh0406 force-pushed the main branch 4 times, most recently from 754f082 to 20bf828 Compare

November 14, 2023 02:47


          Add migrations for shards table in postgres metastore

253c4b0

kamalesh0406 force-pushed the main branch from 20bf828 to 253c4b0 Compare

November 16, 2023 03:57

guilload merged commit 749c170 into quickwit-oss:main

4 checks passed

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

Labels

None yet