Add SpannerMetadataModule with ExtensionLoader #15959

123-komal · 2025-05-29T10:42:53Z

No description provided.

bhardwaj-priyanshu · 2025-05-29T10:44:37Z

cdap-common/src/main/resources/cdap-default.xml

@@ -2845,6 +2845,11 @@
    <value>/opt/cdap/master/ext/log-publisher</value>
  </property>

+  <property>


Can we add a flag to determine if we want to use spanner instead of elastic search?

This is just for extension (not specific to spanner) - I think we might not need a flag here.

Komal is anyways testing the default flow for ES implementation. So should be safe.

bhardwaj-priyanshu · 2025-05-29T10:46:37Z

cdap-data-fabric/src/main/java/io/cdap/cdap/data2/metadata/AuditMetadataStorage.java

-        Constants.Metrics.STORAGE_METRICS_TAGS);
-    metricsCollector.increment(Constants.Metrics.MetadataStorage.METRICS_PREFIX + metricSuffix, 1L);
+            Constants.Metrics.STORAGE_METRICS_TAGS);
+    metricsCollector.increment(Constants.Metrics.MetadataStorage.METRICS_PREFIX + metricSuffix,1L);


why are these lines changed? because of formatting? I think the earlier formatting was correct are you using Google formatter?

bhardwaj-priyanshu · 2025-05-29T10:49:01Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/DefaultMetadataStorageProvider.java

+        this.cConf = cConf;
+        this.extensionLoader = extensionLoader;
+        this.extensionLoader.getAll();
+


remove new line

bhardwaj-priyanshu · 2025-05-29T10:52:26Z

cdap-data-fabric/src/main/java/io/cdap/cdap/data/runtime/DataSetsModules.java

@@ -179,7 +179,7 @@ public MetadataStorage get() {
      return injector.getInstance(DatasetMetadataStorage.class);
    }
    if (Constants.Metadata.STORAGE_PROVIDER_ELASTICSEARCH.equalsIgnoreCase(config)) {
-      return injector.getInstance(ElasticsearchMetadataStorage.class);
+      return injector.getInstance(DefaultMetadataStorageProvider.class);


I see this is loading the extenssion for spanner. i.e. after your change the new instances will not use elastic search is that expected? we dont have any control plane changes ready?

I think we should have a cconf flag to guard this change and when everything is okay we can turn that on from the control plane

bhardwaj-priyanshu · 2025-05-29T10:53:10Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/DefaultMetadataStorageProvider.java

+    @Override
+    public MetadataChange apply(MetadataMutation mutation, MutationOptions options)
+            throws IOException {
+        return getDelegate().apply(mutation, options);


do we really need to call getDelegate everytime or we can cache the result ?

I think it is advisable to call getDelegate() each time as that method is rightly synchronized. Similar thing is done in DefaultStorageProvider and DelegatingMessagingService.

sidhdirenge · 2025-05-29T13:25:58Z

...ta-fabric/src/main/java/io/cdap/cdap/spi/metadata/DefaultMetadataStorageProviderContext.java

+        String propertiesPrefix =
+                Constants.Dataset.STORAGE_EXTENSION_PROPERTY_PREFIX + storageImpl + ".";
+        this.cConf = Collections.unmodifiableMap(cConf.getPropsWithPrefix(propertiesPrefix));
+        this.properties = Collections.unmodifiableMap(cConf.getPropsWithPrefix(propertiesPrefix));


Both these variables are having same values - do we need 2 separate vars here?

sidhdirenge · 2025-05-29T13:27:45Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/dataset/DatasetMetadataStorage.java

 import java.util.stream.Collectors;
-import org.apache.tephra.TransactionSystemClient;
+


Please take care while committing changes - the ones unrelated to your PR should not be changed (even if Google formatter updates few other lines) Otherwise you will mess up the blame / git history for the original changes.

sidhdirenge · 2025-05-29T13:31:01Z

cdap-metadata-ext-spanner/src/main/java/io/cdap/cdap/metadata/spanner/RequestAndChange.java

+ * A simple class to pass around a Spanner Mutation, along with the metadata
+ * change that it effects.
+ */
+public class RequestAndChange implements ChangeRequest {


Usually naming convention is like : For spanner impl you can call this class as SpannerChangeRequest

For ES -> ElasticSearchChangeRequest / ESChangeRequest.

sidhdirenge · 2025-05-29T13:31:35Z

cdap-metadata-spi/src/main/java/io/cdap/cdap/spi/metadata/MetadataStorageContext.java

+import java.util.Map;
+
+public interface MetadataStorageContext {
+    Map<String, String> getProperties();


Please check if this one is needed.

sidhdirenge · 2025-05-29T13:32:22Z

pom.xml

@@ -48,6 +48,9 @@
      <organizationUrl>http://www.cdap.io</organizationUrl>
    </developer>
  </developers>
+  <modules>
+    <module>cdap-metadata-ext-spanner</module>


you can move this to place where other modules are added.

sidhdirenge · 2025-05-30T03:54:39Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/DefaultMetadataStorageProvider.java

+import java.io.IOException;
+import java.util.List;
+
+public class DefaultMetadataStorageProvider implements MetadataStorage {


I am not sure if we should call this as Provider.

DefaultStorageProvider implements StorageProvider (in case of storage tables).
For Metadata, we do not have an interface like MetadataStorageProvider.

Maybe we can call it as DefaultMetadataStorage / DelegatingMetadataStorage?

@bhardwaj-priyanshu - wdyt?

sidhdirenge · 2025-05-30T03:58:07Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/DefaultMetadataStorageProvider.java

+
+    public String getName() {
+        return cConf.get(Constants.Metadata.STORAGE_PROVIDER_SPANNER);
+    }


This should just read the MetadataStorage property from cConf.

This function will check if the property value matches the getName() in ES or getName in Spanner impl.

Or you can even follow something llike this : https://github.com/cdapio/cdap/blob/develop/cdap-data-fabric/src/main/java/io/cdap/cdap/spi/data/common/DefaultStorageProvider.java#L53

and then have switch case like : https://github.com/cdapio/cdap/blob/develop/cdap-data-fabric/src/main/java/io/cdap/cdap/spi/data/common/DefaultStorageProvider.java#L96

Both the cases work.

sidhdirenge · 2025-05-30T04:02:17Z

cdap-metadata-ext-spanner/pom.xml

+        <dependency>
+            <groupId>com.google.cloud</groupId>
+            <artifactId>google-cloud-spanner</artifactId>
+            <version>6.12.1</version>


Please update the spanner version to use latest. Actually the spanner package comes from the google cloud library. You need to override the google cloud library in this extension to use the latest version like this - https://github.com/cdapio/cdap/blob/develop/cdap-storage-ext-spanner/pom.xml#L41.

sidhdirenge · 2025-05-30T04:03:44Z

cdap-metadata-ext-spanner/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>io.cdap.cdap</groupId>
+            <artifactId>cdap-common</artifactId>


We should not have dependency on cdap-common ideally. There is a chance that some mvn packages will have versions incompatible to the latest spanner ones.

Please check if there is a way to remove it.

sidhdirenge · 2025-05-30T12:34:31Z

cdap-common/src/main/resources/cdap-default.xml

@@ -2845,6 +2845,11 @@
    <value>/opt/cdap/master/ext/log-publisher</value>
  </property>

+  <property>


This is just for extension (not specific to spanner) - I think we might not need a flag here.

Komal is anyways testing the default flow for ES implementation. So should be safe.

sidhdirenge · 2025-05-30T12:35:37Z

cdap-data-fabric/src/main/java/io/cdap/cdap/data/runtime/DataSetsModules.java

@@ -179,7 +179,7 @@ public MetadataStorage get() {
      return injector.getInstance(DatasetMetadataStorage.class);
    }
    if (Constants.Metadata.STORAGE_PROVIDER_ELASTICSEARCH.equalsIgnoreCase(config)) {


Please update the if case as discussed earlier.
Let the DefaultStorageProvider handle the if else logic for ES to use or Spanner

sidhdirenge · 2025-05-30T12:37:10Z

cdap-data-fabric/src/main/java/io/cdap/cdap/data2/metadata/AuditMetadataStorage.java

@@ -83,6 +84,16 @@ public void setAuditPublisher(AuditPublisher auditPublisher) {
    this.auditPublisher = auditPublisher;
  }

+  @Override
+  public String getName() {


Please verify how this is called since the default value of metadata.storage.implementation=elastic

sidhdirenge · 2025-05-30T12:37:59Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/DefaultMetadataStorageProvider.java

+            if (metadataStorage != null) {
+                return metadataStorage;
+            }
+            LOG.info(extensionLoader.get(getName()).toString());


Please remove extra loggers that are used for debugging right now.

you will run into NPE if getName is empty string - we should not run into NPEs.

sidhdirenge · 2025-05-30T12:40:31Z

cdap-data-fabric/src/main/java/io/cdap/cdap/spi/metadata/dataset/DatasetMetadataStorage.java

@@ -74,6 +74,16 @@ public class DatasetMetadataStorage extends SearchHelper implements MetadataStor
    super(txClient, tableDefinition);
  }

+  @Override
+  public String getName(){
+    return "DatasetMetadataStorage";


you can instead use .getClass().getSimpleName() instead

sidhdirenge · 2025-05-30T12:43:19Z

cdap-metadata-ext-spanner/pom.xml

+            <scope>compile</scope>
+        </dependency>
+        <dependency>
+            <groupId>com.google.cloud</groupId>


Please update the entire google cloud version
This will eventually be overridden when google cloud package is imported

https://github.com/cdapio/cdap/blob/develop/cdap-storage-ext-spanner/pom.xml#L41

You can try mvn dependency tree command to find out which version is pulled.

sidhdirenge · 2025-05-30T12:44:27Z

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

+
+    @Override
+    public void initialize(MetadataStorageContext context) throws Exception {
+        Log.info("Spanner Initialized");


please use static logger - https://github.com/cdapio/cdap/blob/develop/cdap-tms/src/main/java/io/cdap/cdap/messaging/client/DelegatingMessagingService.java#L47

sidhdirenge · 2025-05-30T12:45:03Z

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

+
+    @Override
+    public String getName() {
+        return Constants.Metadata.STORAGE_PROVIDER_SPANNER;


Probably this is adding the cdap-common dependency.
Please feel free to remove this constant to avoid that dependency.

sidhdirenge · 2025-05-30T12:46:10Z

cdap-elastic/src/main/java/io/cdap/cdap/metadata/elastic/ESChangeRequest.java

I do not see the META-INF for this. Please check - I guess it would be needed for Extension loader.

Format Changes changes Changes name-changes formatting-changes elastic-changes

123-komal requested review from bhardwaj-priyanshu and sidhdirenge May 29, 2025 10:42

bhardwaj-priyanshu reviewed May 29, 2025

View reviewed changes

sidhdirenge reviewed May 29, 2025

View reviewed changes

123-komal force-pushed the es-spanner_feature branch from 366ff67 to fe0dfb3 Compare May 30, 2025 03:42

sidhdirenge reviewed May 30, 2025

View reviewed changes

123-komal force-pushed the es-spanner_feature branch 3 times, most recently from 18e8057 to 05ce05e Compare May 30, 2025 09:46

sidhdirenge reviewed May 30, 2025

View reviewed changes

Add SpannerMetadataModule with ExtensionLoader

b2a0e2b

Format Changes changes Changes name-changes formatting-changes elastic-changes

123-komal force-pushed the es-spanner_feature branch from 05ce05e to b2a0e2b Compare May 30, 2025 16:02

		import java.util.stream.Collectors;
		import org.apache.tephra.TransactionSystemClient;

Add SpannerMetadataModule with ExtensionLoader #15959

Are you sure you want to change the base?

Add SpannerMetadataModule with ExtensionLoader #15959

Uh oh!

Conversation

123-komal commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!