-
Notifications
You must be signed in to change notification settings - Fork 37
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
Support namespace mappings in MultiStorage #207
Support namespace mappings in MultiStorage #207
Conversation
82f5103
to
4ecb62a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good. Thank you!
Left a minor question and comments.
|
||
checkIfStorageExists(storage); | ||
builder.put(table, storage); | ||
if (tableMapping != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a minor style thing, but I feel the following is slightly better since less indenting.
if (tableMapping == null) {
return null;
}
for (String tableAndStorage : tableMapping.split(",")) {
// ...
}
Builder<String, String> builder = ImmutableMap.builder(); | ||
|
||
String namespaceMapping = props.getProperty(NAMESPACE_MAPPING); | ||
if (namespaceMapping != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
} | ||
return storage; | ||
String namespace = operation.forNamespace().get(); | ||
storage = namespaceStorageMap.get(namespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if conflicting mapping like below is given, it prefers namespace mapping, right? Or we shouldn't use both mapping format together?
scalar.db.multi_storage.table_mapping=user.ORDER:cassandra,user.CUSTOMER:mysql,coordinator.state:cassandra
scalar.db.multi_storage.namespace_mapping=user:cassandra
Anyway, I think it would be great if we note it somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing this!
If conflicting mapping between a table mapping and a namespace mapping, it prefers the table mapping because table mappings are more specific than namespace mappings.
Sure, I will add it to the Javadoc. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I meant table mapping is preferred. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
I left minor suggestions on the English comments.
} | ||
return storage; | ||
String namespace = operation.forNamespace().get(); | ||
storage = namespaceStorageMap.get(namespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I meant table mapping is preferred. Thank you!
src/main/java/com/scalar/db/storage/multistorage/MultiStorage.java
Outdated
Show resolved
Hide resolved
src/main/java/com/scalar/db/storage/multistorage/MultiStorageAdmin.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Hiroyuki Yamada <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT!
Currently, we only have a table mapping support in MultiStorage as follows:
This PR adds a namespace mapping support where we can define namespace mappings as follows:
Please take a look!