[CDAP-21172]Added cdap-metadata-ext-spanner Module with ExtensionLoader#15959
Conversation
| 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); |
There was a problem hiding this comment.
why are these lines changed? because of formatting? I think the earlier formatting was correct are you using Google formatter?
| this.cConf = cConf; | ||
| this.extensionLoader = extensionLoader; | ||
| this.extensionLoader.getAll(); | ||
|
|
There was a problem hiding this comment.
remove new line
| } | ||
| if (Constants.Metadata.STORAGE_PROVIDER_ELASTICSEARCH.equalsIgnoreCase(config)) { | ||
| return injector.getInstance(ElasticsearchMetadataStorage.class); | ||
| return injector.getInstance(DefaultMetadataStorageProvider.class); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We did that previously also at that time it worked
There was a problem hiding this comment.
but it worked if use just ElasticsearchMetadataStorage so I think it is issue
| @Override | ||
| public MetadataChange apply(MetadataMutation mutation, MutationOptions options) | ||
| throws IOException { | ||
| return getDelegate().apply(mutation, options); |
There was a problem hiding this comment.
do we really need to call getDelegate everytime or we can cache the result ?
There was a problem hiding this comment.
I think it is advisable to call getDelegate() each time as that method is rightly synchronized. Similar thing is done in DefaultStorageProvider and DelegatingMessagingService.
123-komal
force-pushed
the
es-spanner_feature
branch
from
366ff67 to
fe0dfb3
Compare
123-komal
force-pushed
the
es-spanner_feature
branch
3 times, most recently
from
18e8057 to
05ce05e
Compare
123-komal
force-pushed
the
es-spanner_feature
branch
4 times, most recently
from
b2a0e2b to
682799c
Compare
|
|
||
| public class DefaultMetadataStorageProviderContext implements MetadataStorageContext { | ||
|
|
||
| private static final String storageImpl = "gcp-spanner"; |
There was a problem hiding this comment.
this should be coming from the cconf I feel
| } | ||
| }; | ||
| } | ||
| } No newline at end of file |
| if (metadataStorage != null) { | ||
| return metadataStorage; | ||
| } | ||
| synchronized (this) { |
There was a problem hiding this comment.
do we need to synchronize on this? cant we do only on MetadataStorage.class?
something similar to
if (metadataStorage == null) {
synchronized (MetadataStorage.class) {
if (metadataStorage == null) {
metadataStorage = extensionLoader.get(getName()));
// and other stuffs
}
}
}
@sidhdirenge wdyt?
There was a problem hiding this comment.
For now I would leave it as is - this is the pattern followed by other Providers.
We might miss some race condition maybe?
123-komal
force-pushed
the
es-spanner_feature
branch
from
682799c to
35568f8
Compare
sidhdirenge
left a comment
sidhdirenge
left a comment
There was a problem hiding this comment.
We are almost there with the changes! Before your next revision please make sure you do the following :
- Copyright for all the new files added. I would suggest run the mvn commands without
-Drat.skip=true. You will be able to catch all the files with missing copyrights (for sanity check) - Proper google formatting - Please make sure you do
ctrl+alt+L. Do this consecutively twice - to update the line breaks as well. - Java docs for all the new classes and all the public methods.
123-komal
force-pushed
the
es-spanner_feature
branch
from
35568f8 to
0607c2d
Compare
123-komal
force-pushed
the
es-spanner_feature
branch
from
2b9e276 to
5d0177d
Compare
| public class DelegatingMetadataStorage implements MetadataStorage { | ||
| private final CConfiguration cConf; | ||
| private final MetadataStorage delegate; | ||
| private static String prefix = ""; |
There was a problem hiding this comment.
initialization should be done in constructor.
| } | ||
| // TODO(CDAP-21174): Create metadata storage specific properties | ||
| if (getName().equals("gcp-spanner")) { | ||
| prefix = Constants.Dataset.STORAGE_EXTENSION_PROPERTY_PREFIX + getName() + "."; |
There was a problem hiding this comment.
use String.format("%s%s.", Constants.Dataset.STORAGE_EXTENSION_PROPERTY_PREFIX, getName())
123-komal
force-pushed
the
es-spanner_feature
branch
from
5d0177d to
60e25fa
Compare
| this.prefix = String.format("%s%s.", Constants.Dataset.STORAGE_EXTENSION_PROPERTY_PREFIX, getName()); | ||
| } | ||
| else{ | ||
| this.prefix=""; |
| if (getName().equals("gcp-spanner")) { | ||
| this.prefix = String.format("%s%s.", Constants.Dataset.STORAGE_EXTENSION_PROPERTY_PREFIX, getName()); | ||
| } | ||
| else{ |
There was a problem hiding this comment.
add space after else
Can you please use Google Configuration Checker Style? It will automatically take care of these issues.
123-komal
force-pushed
the
es-spanner_feature
branch
from
60e25fa to
604d785
Compare
|
|
||
| private final Map<String, String> properties; | ||
|
|
||
| DefaultMetadataStorageContext(CConfiguration cConf, String prefix) { |
There was a problem hiding this comment.
please add @Nullable annotation for nullable values which is prefix here.
| private final Map<String, String> properties; | ||
|
|
||
| DefaultMetadataStorageContext(CConfiguration cConf, String prefix) { | ||
| this.properties = Collections.unmodifiableMap(cConf.getPropsWithPrefix(prefix)); |
There was a problem hiding this comment.
Also if prefix is null initialize properties with emptyMap otherwise cConf.getPropsWithPrefix(prefix) will throw NPE
| throw new IllegalArgumentException("Unsupported MetadataProvider type: " + getName()); | ||
| } | ||
| // TODO(CDAP-21174): Create metadata storage specific properties | ||
| if (getName().equals("gcp-spanner")) { |
There was a problem hiding this comment.
please define a private static final variable for this constant.
There was a problem hiding this comment.
for private static final it needs to be initialized there and as you said early it shouldn't be defined outside the constructor.
There was a problem hiding this comment.
I meant to add it for string gcp-spanner constant.
123-komal
force-pushed
the
es-spanner_feature
branch
from
604d785 to
a59bbb0
Compare
| private final Map<String, String> properties; | ||
|
|
||
| DefaultMetadataStorageContext(CConfiguration cConf, @Nullable String prefix) { | ||
| if(prefix!=null){ |
There was a problem hiding this comment.
checkstyle looks off, add space after )
There was a problem hiding this comment.
add space before and after !=
| // TODO(CDAP-21174): Create metadata storage specific properties | ||
|
|
||
| /** | ||
| * Default implementation of the {@link MetadataStorageContext}. | ||
| */ |
There was a problem hiding this comment.
please combine the top level single line comment in multi-line javadoc.
123-komal
force-pushed
the
es-spanner_feature
branch
2 times, most recently
from
84d66ee to
5190754
Compare
|
|
||
|
|
||
| /** | ||
| * // TODO(CDAP-21174): Create metadata storage specific properties |
123-komal
force-pushed
the
es-spanner_feature
branch
from
5190754 to
e5a08bb
Compare
|
|
||
|
|
||
| /** | ||
| * // TODO(CDAP-21174): Create metadata storage specific properties |
There was a problem hiding this comment.
nit :
- no need of
//before TODO. - In any java docs, the description should come first. Then you can have the TODO (followed by arguments/return vars in case of Java docs for methods). You can refer to other places in the codebase.
| private final CConfiguration cConf; | ||
| private final MetadataStorage delegate; | ||
| private static String prefix; | ||
| private static final String metadatastorageImpl = "gcp-spanner"; |
There was a problem hiding this comment.
Please use something like : METDATA_STORAGE_IMPL
also -
I think metadataStorageImpl is very vague term here. It's value is very spanner specific.
Would be better to rename it.
123-komal
force-pushed
the
es-spanner_feature
branch
from
e5a08bb to
35dd43f
Compare
123-komal
force-pushed
the
es-spanner_feature
branch
from
35dd43f to
813e666
Compare
Format Changes changes Changes name-changes formatting-changes elastic-changes changes Added copyright section Updated Updated Added Javadocs Corrected Copyright Corrected Copyright Lexicographical order correction Formatting correction Formatting correction changes Updated PR Updated names Updated comments Updated commit Formatting changes Formatting changes Formatting changes Formatting Changes Prefix Change Formatting Changes Formatting Changes Format changes edited default context edited default context added constant Formatting Changes Formatting Changes Corrected Formatting Corrected Formatting
123-komal
force-pushed
the
es-spanner_feature
branch
from
813e666 to
14ce5fc
Compare
|
5 participants
Description:
This PR introduces the
SpannerMetadataStorageand it is being loaded at runtime using theMetadataStorageExtensionLoader. Key changes include:SpannerMetadataStorage: A new class that extends
MetadataStoragethat handles the pipeline metadata and stores it in spanner tables. It delegates the operations (such as createIndex, apply, and search) to the spanner tables in the spanner instance.DelegatingMetadataStorage: This class acts as a wrapper around
SpannerMetadataStorage, enabling the use ofSpannerMetadataStoragewithin theMetadataStorageframework. It ensures that the SpannerMetadataStorage is properly initialized.Testing: