[CDAP-21172] Add Creation/Deletion logic for Metadata Tables by 123-komal · Pull Request #15971 · cdapio/cdap

123-komal · 2025-06-09T13:25:07Z

Description:

This PR is the follow-up for #15959 and here we are temporarily building this module separately as a workaround for a dependency issue that breaks the unified build and the unit-tests will be added in the follow-up PRs. It's key changes are listed below:

SpannerMetadataStorage: In this PR createIndex has been implemented along with its helper methods.This change is responsible for the creation of tables in the spanner instance.It also implements the dropIndex method which is responsible for dropping the tables in spanner instance.
SpannerConfig:This class is responsible for setting up the spanner-related configurations.

Testing:

Local Testing:Tested these changes using a Distributed docker image.

sidhdirenge

We are missing one important thing here - table updates. Updates would need some extra checks like schema backward compatibility, no new insertion of Primary keys etc.

This is out of scope for this PR but we can add a TODO related to handling schema updates.

sidhdirenge · 2025-06-10T12:37:44Z

+  static final String PROJECT = "project";
+  static final String INSTANCE = "instance";
+  static final String DATABASE = "database";


can we make these fields private?

We will be using these fields for unit tests so we use them or we can add getter methods for them.

We can mock it. It should not be difficult

sidhdirenge · 2025-06-10T12:41:19Z


+  private static final Logger LOG = LoggerFactory.getLogger(SpannerMetadataStorage.class);
+
+  private static final String METADATA_TABLE = "metadata"; // Table name


no need of line comments to explain the purpose of variables.

You can instead divide these variables into sections and add comments like:

// Metadata storage tables private static final String METADATA_TABLE = "metadata"; private static final String METADATA_PROPS_TABLE = "metadata_props"; // `metadata` table fields . . . // `metadata_props` table fields . .

sidhdirenge · 2025-06-10T12:41:35Z

+  private static final String TYPE_FIELD = "entity_type"; // Type of the entity (e.g., dataset, application)
+  private static final String NAME_FIELD = "name"; // Name of the entity
+  private static final String CREATED_FIELD = "creation_time"; // creation-time
+  private static final String User_FIELD = "user"; // USER-scoped properties/tags


rename to USER_FIELD

sidhdirenge · 2025-06-10T12:43:55Z

+  private String instanceId = "instance";
+  private String databaseId = "database";
+  private String projectId = "project";


Why are we initializing these values?

+1 this should be configured by cdapmaster

sidhdirenge · 2025-06-10T13:09:16Z

+      LOG.error("Error executing DDL statements: {}", e.getMessage(), e);
+      throw new IOException(e);


no need of extra logger here - IOException will print the entire stack trace in the logs.

sidhdirenge · 2025-06-10T13:10:08Z

  @Override
  public void dropIndex() throws IOException {
-    throw new IOException("NOT IMPLEMENTED");
+    synchronized (this) {


No need of synchronization here. You can just silently ignore exception if table does not exist.

sidhdirenge · 2025-06-10T13:10:29Z

+      statements.add(String.format("DROP TABLE IF EXISTS %s", METADATA_PROPS_TABLE));
+      try {
+        executeCreateDDLStatements(statements);
+        LOG.info("Table dropped successfully.");


Logger not very informational - which table was dropped?

sidhdirenge · 2025-06-10T13:10:47Z

+        executeCreateDDLStatements(statements);
+        LOG.info("Table dropped successfully.");
+      } catch (IOException e) {
+        LOG.error("Error dropping index: {}", e.getMessage(), e);


No need of extra logger here.

sidhdirenge · 2025-06-10T13:21:10Z

+  }
+
+  /**
+   * <p>Key features of the schema:


Feel free to add some examples in the javadocs like we have done in messaging - https://github.com/cdapio/cdap/blob/develop/cdap-messaging-ext-spanner/src/main/java/io/cdap/cdap/messaging/spanner/SpannerMessagingService.java#L304

Whenever someone will see this code for the first time, it will be easy for them to understand why 2 tables were needed here.

sidhdirenge · 2025-06-10T13:30:22Z

+        "Value_Tokens TOKENLIST AS " +
+        "(TOKENIZE_SUBSTRING(Props_Value, ngram_size_min=>1, ngram_size_max=>3)) HIDDEN," +
+        ")PRIMARY KEY (metadata_id, %s, %s) ," + // primary key
+        "INTERLEAVE IN PARENT metadata ON DELETE CASCADE",


metadata can be replaced with table name defined at the start.

sidhdirenge · 2025-06-10T13:31:46Z

+    LOG.info("Generating create table DDL statement.");
+    return String.format(
+      "CREATE TABLE IF NOT EXISTS %s (" +
+        "Metadata_ID STRING(MAX) NOT NULL," +


metadata_id can also be defined as another constant with field name.

sidhdirenge · 2025-06-10T13:33:04Z

+        "%s STRING(MAX)," + // system
+        "metadata_column JSON," + // metadata
+        "VERSION INT64 NOT NULL," +
+        "User_Tokens TOKENLIST AS " +


column names should always be -> user_tokens, system_tokens, text_tokens

bhardwaj-priyanshu · 2025-06-10T14:27:29Z

+    }
+    return cConf.get(PROJECT);
+  }
+}


Add new line

bhardwaj-priyanshu · 2025-06-10T14:30:08Z

@@ -16,6 +16,15 @@

 package io.cdap.cdap.metadata.spanner;



Can we add test file for this class?

cc @sidhdirenge

bhardwaj-priyanshu · 2025-06-10T14:43:12Z

+    String instance = cConf.get(DATABASE);
+    if (instance == null) {
+      throw new IllegalArgumentException("Missing configuration " + DATABASE);
+    }


why the variable name is instance?

When we already have the data in instance variable why are we fetching that again?

Cant we use Objects.requireNonNull() function that will throw the null pointed exception simply?

bhardwaj-priyanshu · 2025-06-10T14:46:00Z

+  }
+
+  static String getProjectID(Map<String, String> cConf) {
+    String instance = cConf.get(PROJECT);


why the variable name is instance?

When we already have the data in instance variable why are we fetching that again?

Cant we use Objects.requireNonNull() function that will throw the null pointed exception simply?

bhardwaj-priyanshu · 2025-06-10T14:50:05Z

+    if (created) {
+      return;
+    }
+    synchronized (this) {


Avoid synchronizing on this. It locks the entire object and can block threads unnecessarily. Instead, if needed synchronize only on the specific objects that require protection.

sidhdirenge · 2025-06-11T10:39:53Z

+  private static final String METADATA_ID_FIELD = "metadata_id"; // metadata_id of the metadata
+  private static final String METADATA_COLUMN_FIELD = "metadata_column"; // contains the metadata
+  private static final String NAMESPACE_FIELD = "namespace"; // namespace of the metadata
+  private static final String TYPE_FIELD = "entity_type"; // type of the entity (e.g., dataset, application)
+  private static final String NAME_FIELD = "name"; // name of the entity
+  private static final String VERSION = "version";
+  private static final String CREATED_FIELD = "creation_time"; // creation-time
+  private static final String USER_FIELD = "user"; // USER-scoped properties/tags
+  private static final String SYSTEM_FIELD = "system"; // SYSTEM-scoped properties/tags


It is okay to remove the line comments here. They are not adding significant value.

sidhdirenge · 2025-06-11T10:41:47Z

+      executeDdlStatements(statements);
+      LOG.info("metadata and metadata_props dropped successfully.");
+    } catch (IOException e) {
+      LOG.error("Error dropping index: {}", e.getMessage(), e);


no need of extra logger here.

updated addressed comments

sonarqubecloud · 2025-06-13T10:25:18Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

123-komal force-pushed the create_branch branch 3 times, most recently from 51d0b07 to ef67afc Compare June 10, 2025 10:57

123-komal changed the title ~~Create branch~~ [CDAP-21172] Implemented createIndex and dropIndex method Jun 10, 2025

123-komal force-pushed the create_branch branch from 2af17c4 to cc4785a Compare June 10, 2025 11:16

123-komal requested review from bhardwaj-priyanshu and sidhdirenge June 10, 2025 11:17

sidhdirenge reviewed Jun 10, 2025

View reviewed changes

bhardwaj-priyanshu reviewed Jun 10, 2025

View reviewed changes

Comment thread cdap-metadata-ext-spanner/src/main/java/io/cdap/cdap/metadata/spanner/SpannerConfig.java Outdated

bhardwaj-priyanshu reviewed Jun 10, 2025

View reviewed changes

Comment thread ...metadata-ext-spanner/src/main/java/io/cdap/cdap/metadata/spanner/SpannerMetadataStorage.java

123-komal force-pushed the create_branch branch 7 times, most recently from 94e5d3b to f0b7cc5 Compare June 11, 2025 08:55

123-komal changed the title ~~[CDAP-21172] Implemented createIndex and dropIndex method~~ [CDAP-21172] Implement createIndex and dropIndex method Jun 11, 2025

123-komal force-pushed the create_branch branch from f0b7cc5 to 8b8683e Compare June 11, 2025 10:38

sidhdirenge reviewed Jun 11, 2025

View reviewed changes

123-komal force-pushed the create_branch branch from 8b8683e to beb9beb Compare June 11, 2025 14:18

123-komal changed the title ~~[CDAP-21172] Implement createIndex and dropIndex method~~ [CDAP-21172] Add Creation/Deletion logic for Metadata Tables Jun 11, 2025

vsethi09 reviewed Jun 11, 2025

View reviewed changes