-
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
Refactor the code around metadata #199
Conversation
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 1 question and 1 minor comment.
|
||
/** | ||
* A metadata class for a table of Scalar DB to know the type of each column | ||
* | ||
* @author Yuji Ito | ||
*/ | ||
public class CosmosTableMetadata implements TableMetadata { | ||
public class CosmosTableMetadata { |
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.
Just a question. Why this cannot be unified to TableMetadata
?
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!
We still need CosmosTableMetadata
because we need a POJO (that means a class with some fields and their getters and setters maybe) to serialize and deserialize data in Cosmos DB.
For example, the following code writes the metadata into Cosmos DB with CosmosTableMetadata
:
scalardb/src/integration-test/java/com/scalar/db/storage/cosmos/CosmosIntegrationTest.java
Lines 85 to 100 in 2ff0ff3
CosmosTableMetadata metadata = new CosmosTableMetadata(); | |
metadata.setId(table(NAMESPACE, TABLE)); | |
metadata.setPartitionKeyNames(Collections.singletonList(COL_NAME1)); | |
metadata.setClusteringKeyNames(Collections.singletonList(COL_NAME4)); | |
metadata.setSecondaryIndexNames(new HashSet<>(Collections.singletonList(COL_NAME3))); | |
Map<String, String> columns = new HashMap<>(); | |
columns.put(COL_NAME1, "int"); | |
columns.put(COL_NAME2, "text"); | |
columns.put(COL_NAME3, "int"); | |
columns.put(COL_NAME4, "int"); | |
columns.put(COL_NAME5, "boolean"); | |
metadata.setColumns(columns); | |
client | |
.getDatabase(database(METADATA_DATABASE)) | |
.getContainer(METADATA_CONTAINER) | |
.createItem(metadata); |
And the following code retrieves the metadata from Cosmos DB with CosmosTableMetadata
:
scalardb/src/main/java/com/scalar/db/storage/cosmos/CosmosTableMetadataManager.java
Lines 49 to 51 in 2ff0ff3
return container | |
.readItem(fullName, new PartitionKey(fullName), CosmosTableMetadata.class) | |
.getItem(); |
As the new TableMetadata
is not a POJO, we can't use it for that.
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! I see.
Since CosmosTableMetadata
seems only used in CosmosTableMetadataManager
, can it be an inner class of the manager class?
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.
CosmosTableMetadata
is used only in CosmosTableMetadataManager
in the main code, but it's actually used in the integration test code as well. And we might use this class another part when we introduce DistributedStorageAdmin
. So I want to keep it an outer class for now. If we realize it doesn't need to be an outer class in the future, we can do that at that time. 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.
OK, I understand.
import java.util.Objects; | ||
import java.util.Set; | ||
|
||
/** Represents the table metadata */ |
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's not in the google style guide, but I think it would be a bit better and consistent if a comment for a class starts with a noun.
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! Thank you!
@yito88 I merged it but since it's a bit big PR so please take a look when you get a chance. |
I wanted to add a
DistributedStorageAdmin
interface that gets table metadata and creates/drops tables. But, before that, we need to do some refactoring for the code around metadata. This PR is for this.In this change, I removed the existing
TableMetadata
interface and added the newTableMetadata
class with the builder class. Because of this, we can removeCassandraTableMetadata
andDynamoTableMetadata
andJdbcTableMetadata
(CosmosTableMetadata
is still needed though).After it's merged, I want to add
DistributedStorageAdmin
like the following:Please take a look at this!