Skip to content

Add configuration options to control table creation restrictions #1503

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

platinumhamburg
Copy link
Contributor

Purpose

Linked issue: close #1502

Brief change log

Tests

API and Format

Documentation

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

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

Thanks @platinumhamburg for contribution.

Please add tests for this feature (e.g., in FlussAdminITCase or TableManagerITCase). Besides, please add documentation for the new configs.

@@ -1309,6 +1309,24 @@ public class ConfigOptions {
"The column name of the version column for the `versioned` merge engine. "
+ "If the merge engine is set to `versioned`, the version column must be set.");

public static final ConfigOption<Boolean> LOG_TABLE_ALLOW_CREATION =
key("log.table.allow-creation")
Copy link
Member

Choose a reason for hiding this comment

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

How about allow.create.log.tables and allow.create.kv.tables? This makes sure both of them be able to be together on docs when we support auto generate docs in the future.

And please put the ConfigOptions after AUTO_PARTITION_CHECK_INTERVAL? Because table.* are table-level configs, not server-side configs.

Comment on lines +677 to +679
"Creation of KV tables (primary key tables) is not allowed. "
+ "Table: %s. Please set '%s' to true to enable KV table creation.",
tablePath, ConfigOptions.KV_TABLE_ALLOW_CREATION.key()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Creation of KV tables (primary key tables) is not allowed. "
+ "Table: %s. Please set '%s' to true to enable KV table creation.",
tablePath, ConfigOptions.KV_TABLE_ALLOW_CREATION.key()));
"Creation of Primary Key Tables is disallowed in the cluster.");
  1. Use Primary Key Table for user-facing messages, we don't have the concept of KV table actually.
  2. remove the cluster config from the exception message, because user shouldn't be aware of the cluster config and usually don't have the permission to access cluster configs (so it's not necessary to expose them).

Comment on lines +684 to +688
throw new InvalidTableException(
String.format(
"Creation of Log tables is not allowed. "
+ "Table: %s. Please set '%s' to true to enable Log table creation.",
tablePath, ConfigOptions.LOG_TABLE_ALLOW_CREATION.key()));
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Suggested change
throw new InvalidTableException(
String.format(
"Creation of Log tables is not allowed. "
+ "Table: %s. Please set '%s' to true to enable Log table creation.",
tablePath, ConfigOptions.LOG_TABLE_ALLOW_CREATION.key()));
throw new InvalidTableException(
"Creation of Log Tables is disallowed in the cluster.");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement table type creation restrictions via configuration options
2 participants