jtuglu-netflix commented on PR #18082: URL: https://github.com/apache/druid/pull/18082#issuecomment-2969607160
> But since we are doing the ALTERs in a transaction, I guess we should be okay 🤷🏻 . > > Downgrading after that would require the following steps (essentially a revert of all the ALTERs we do this in patch): > > * Ensure that there is only one record per datasource > * Get rid of the extra supervisors for any datasource > * DROP the index on supervisor_id and DROP that column > * ADD the PRIMARY KEY on datasource > > I think this might become simpler if the new PRIMARY KEY is a composite on the two columns (dataSource + supervisor_id). Then, we wouldn't need to do any DB migration while doing a downgrade. > > @jtuglu-netflix , @maytasm , thoughts? > > > > **Edit:** This would essentially play out as follows: > > **Migration for upgrade:** In a transaction: > > * ADD column supervisor_id NOT NULL DEFAULT empty > * ADD UNIQUE INDEX (datasource, supervisor_id) > * DROP PRIMARY KEY (datasource) > > **Migration for downgrade** None > > **CRUD on table** > > * Always do CRUD using both datasource and supervisor_id for consistency and to leverage the UNIQUE INDEX. > * Handle empty supervisor_id values appropriately > (alternative is do an UPDATE in the migration but we can never be sure if the old Overlord hasn't added a new record > with supervisor_id = empty). Agreed, although this confuses the search logic then a bit, since new overlord will need to be looking to upsert on `supervisor_id` = <supervisor id> or `''`. I don't think there's a "clean" way to expose this while having reads/writes going on with different schema versions. The way I see it, there are a few options: 1. We do a "soft" rename of the `dataSource` column, performing no DB schema changes. Pros: - This would ensure forward/backwards compatibility in a rolling upgrade, since both versions of the Overlord would share the same PK and the same write semantics. - No schema changes - No additional changes to the code (yet) Cons: - Muddies the meaning of the `dataSource` column, where some `SeekableStreamSupervisor` types would be writing their `id=<supervisor id>` (which doesn't need to be equal to datasource) and others (like `MaterializedViewSupervisor`) would be writing with their `id=<dataSource>`. 2. We could try to add a `supervisor_id` column that defaults to `''` and have: - writes: set `dataSource`=<dataSource> and `supervisor_id` = <supervisor_id> only if it is not equal to the <dataSource> - reads: Use the `''` clause to determine whether to use the `dataSource` column or not for looking up the exact row. Pros: - We can do a "proper" schema update. Cons: - Doesn't guarantee full backwards/forwards compatibility, consider the case: ``` 1. Old overlord A running 2. Spin up new overlord B with new version 3. Get B elected (A dies, etc.) 4. Create 2 new supervisor 1,2 different from datasource D 5. B dies, etc. 6. A picks up and starts writing metadata to arbitrary rows (they both have same datasource) ``` 3. Create a new, better-named table with future-proof schema - Create a new supervisor metadata table which can have all the new schema, etc. that we need - We can have the code perform reads on old/new table and writes on new table only. Then in a minor version release, cut a version which removes the reads on the old table (not dropping it). Pros: - Can fix the schema/table issues completely Cons: - Have to spread the changes over ≥ 2 versions to allow for safety -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
