Skip to content
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

Introduce system namespace #1345

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Introduce system namespace #1345

merged 5 commits into from
Dec 7, 2023

Conversation

brfrn169
Copy link
Collaborator

@brfrn169 brfrn169 commented Dec 5, 2023 •
edited
Loading

Description

This PR introduces a system namespace that's for internal use, for example the namespace for metadata tables. The default system namespace name is scalardb, but users can change the system namespace name by changing the configuration scalar.db.system_namespace_name.

Related issues and/or PRs

N/A

Changes made

  • Added the configuration scalar.db.system_namespace_name
  • Use the system namespace for the namespace for metadata tables in each adapter
  • Insert the system namespace name to the namespaces metadata table

Checklist

  • 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)

N/A

Release notes

N/A

@brfrn169 brfrn169 added the enhancement New feature or request label Dec 5, 2023
@brfrn169 brfrn169 self-assigned this Dec 5, 2023
brfrn169
brfrn169 commented Dec 5, 2023
View reviewed changes
core/src/main/java/com/scalar/db/config/DatabaseConfig.java
@@ -53,6 +54,9 @@ public class DatabaseConfig {
public static final String CROSS_PARTITION_SCAN = SCAN_PREFIX + "enabled";
public static final String CROSS_PARTITION_SCAN_FILTERING = SCAN_PREFIX + "filtering.enabled";
public static final String CROSS_PARTITION_SCAN_ORDERING = SCAN_PREFIX + "ordering.enabled";
public static final String SYSTEM_NAMESPACE_NAME = PREFIX + "system_namespace_name";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a new config for the system namespace name. The default is scalardb.

core/src/main/java/com/scalar/db/storage/cassandra/CassandraConfig.java

public CassandraConfig(DatabaseConfig databaseConfig) {
String storage = databaseConfig.getStorage();
if (!storage.equals(STORAGE_NAME)) {
throw new IllegalArgumentException(
DatabaseConfig.STORAGE + " should be '" + STORAGE_NAME + "'");
}
metadataKeyspace = getString(databaseConfig.getProperties(), METADATA_KEYSPACE, null);
metadataKeyspace = databaseConfig.getSystemNamespaceName();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use the system namespace for metadata tables in each adapter.

core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java
.addPartitionKey(NAMESPACES_NAME_COL, com.datastax.driver.core.DataType.text())
.getQueryString();
clusterManager.getSession().execute(createTableQuery);

// Insert the system namespace to the namespaces table
upsertIntoNamespacesTable(metadataKeyspace);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, insert the system namespace to the namespaces metadata table. So we can see the system namespace when we call admin.getNamespaceNames().

komamitsu
komamitsu reviewed Dec 6, 2023
View reviewed changes
core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoEnv.java Outdated
@@ -45,7 +45,8 @@ public static Properties getProperties(String testName) {

// Add testName as a metadata namespace suffix
props.setProperty(
DynamoConfig.METADATA_NAMESPACE, DynamoAdmin.METADATA_NAMESPACE + "_" + testName);
DatabaseConfig.SYSTEM_NAMESPACE_NAME,
DatabaseConfig.DEFAULT_NAMESPACE_NAME + "_" + testName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
DatabaseConfig.DEFAULT_NAMESPACE_NAME + "_" + testName);
DatabaseConfig.DEFAULT_SYSTEM_NAMESPACE_NAME + "_" + testName);

I guess you didn't intend to set scalar.db.system_namespace_name property to scalar.db.default_namespace_name_${testName}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! Fixed in efd4c49. Thanks.

@brfrn169 brfrn169 requested a review from komamitsu December 6, 2023 07:59
komamitsu
komamitsu approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@feeblefakie
Copy link
Contributor

@brfrn169 Seems like some of the CIs failed. Can you check?

feeblefakie
feeblefakie reviewed Dec 6, 2023
View reviewed changes
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.

Left one question. PTAL!

core/src/integration-test/java/com/scalar/db/storage/cassandra/CassandraEnv.java
props.setProperty(DatabaseConfig.CROSS_PARTITION_SCAN_FILTERING, "false");
props.setProperty(DatabaseConfig.CROSS_PARTITION_SCAN_ORDERING, "false");
return props;
Properties properties = new Properties();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering why props is renamed to properties.
It's fine, but other XXXEnv don't do the same, I feel it's a bit inconsistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's true. Let me make things consistent. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 7b053c5. Please take a look again when you have time!

josh-wong
josh-wong approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@josh-wong josh-wong 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 requested a review from feeblefakie December 6, 2023 15:08
@brfrn169 brfrn169 marked this pull request as draft December 7, 2023 00:40
@brfrn169 brfrn169 force-pushed the introduce-system-namespace branch from 246d487 to c83c95d Compare December 7, 2023 02:46
@brfrn169 brfrn169 marked this pull request as ready for review December 7, 2023 03:06
feeblefakie
feeblefakie approved these changes Dec 7, 2023
View reviewed changes
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!

@feeblefakie feeblefakie merged commit 0f0c594 into master Dec 7, 2023
23 checks passed
@feeblefakie feeblefakie deleted the introduce-system-namespace branch December 7, 2023 04:20
@brfrn169 brfrn169 mentioned this pull request Dec 7, 2023
Merged
6 tasks
@brfrn169
Copy link
Collaborator Author

brfrn169 commented Dec 7, 2023

@Torch3333 It's merged, but please take a look when you are back!

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

@josh-wong josh-wong josh-wong approved these changes

@feeblefakie feeblefakie feeblefakie approved these changes

@komamitsu komamitsu komamitsu approved these changes

@Torch3333 Torch3333 Awaiting requested review from Torch3333

Assignees

@brfrn169 brfrn169

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brfrn169 @feeblefakie @komamitsu @josh-wong