Skip to content

Conversation

@komamitsu
Copy link
Contributor

@komamitsu komamitsu commented Jul 5, 2024

Description

We added the group commit feature for the coordinator table commit in version 3.13 and 4.x. It added a new column tx_child_ids in coordinator.state table. So, our plan was as follows:

  • In version 3, users manually need to drop and re-create the coordinator table if they want to use the group commit feature
  • In version 4, users can migrate their coordinator table to the new schema that contains tx_child_ids using schema-loader --upgrade command

But, we noticed schema-loader --repair-all --coordinator command with JDBC storage updates the ScalarDB metadata table by adding tx_child_ids column while the existing coordinator table in underlying RDB is left as-is. It results in the inconsistent state and will cause an issue that schema-loader --upgrade wouldn't work with version 4.

After some discussions, we changed our plan for version 3 as follows:

  • If the group commit feature is disabled (default), the old coordinator table schema is used. No issue happens even if schema-loader --repair-all is executed. So, most users don't need to consider the issue.
  • If the group commit feature is enabled, users need to drop and re-create their coordinator table if it exists in advance.
    This must be documented in another PR later

Related issues and/or PRs

None

Changes made

  • Made ConsensusCommitAdmin use the existing coordinator table schema that doesn't have tx_child_idx column if the group commit feature is disabled, and use the new coordinator table schema that has tx_child_idx column if the group commit feature is enabled
  • Updated and added related unit tests and integration tests

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

None

Release notes

N/A

komamitsu added 3 commits July 5, 2024 16:46
based on whether the coordinator table group commit is enabled
@komamitsu komamitsu marked this pull request as ready for review July 5, 2024 10:04
@komamitsu komamitsu self-assigned this Jul 5, 2024
@komamitsu komamitsu added the enhancement New feature or request label Jul 5, 2024
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

}

@Override
protected void extraCheckOnCoordinatorTable() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default check doesn't work. Maybe it's related to

// Share the ClusterManager so that the keyspace metadata stay consistent between the Admin and
// AdminTestUtils
admin = new CassandraAdmin(clusterManager, new DatabaseConfig(properties));
adminTestUtils = new CassandraAdminTestUtils(properties, clusterManager);
...?

@komamitsu komamitsu changed the title Use a proper coordinator table schema Use a proper coordinator table schema based on whether the group commit feature is enabled or not Jul 5, 2024
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@brfrn169 brfrn169 merged commit 2deb11f into 3 Jul 5, 2024
@brfrn169 brfrn169 deleted the use-old-coord-table-metadata-without-groupcommit branch July 5, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants