Skip to content

Conversation

@elek
Copy link
Member

@elek elek commented Mar 19, 2020

What changes were proposed in this pull request?

Description

The MetadataStore interface provides a generic view to any key / value store with a LevelDB and RocksDB implementation.

Since the early version of MetadataStore we also go the DBStore interface which is more andvanced (it supports DB profiles and ColumnFamilies).

To simplify the introduction of new features (like versioning or rocksdb tuning) we should use the new interface everywhere instead of the old interface.

We should update SCM and Datanode to use the DBStore instead of MetadataStore.

This patch is the first part of the cleanup it starts to use DBStore in SCM.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3172

How was this patch tested?

Execution full integration test suite.

@elek elek force-pushed the HDDS-3172 branch 2 times, most recently from 90eee2c to f3e4ac2 Compare March 23, 2020 17:58
Copy link
Contributor

@avijayanhwx avijayanhwx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elek Thank you for working in this involved patch. A few comments inline.

@elek elek changed the base branch from master to master-stable March 27, 2020 10:01
@elek elek changed the base branch from master-stable to master March 30, 2020 13:13
@elek elek changed the title HDDS-3172 Use DBStore instead of MetadataStore in SCM HDDS-3172. Use DBStore instead of MetadataStore in SCM Apr 1, 2020
@adoroszlai
Copy link
Contributor

@elek I think the latest commit (4a8721c) was intended for #578

Copy link
Contributor

@nandakumar131 nandakumar131 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the change looks good to me. Few minor clarifications/comments.

  • DBDefinition is a generic interface which should not have anything specific to particular db. It's using DBStoreBuilder which is specific to rocksdb.
  • SCMMetadataStoreRDBImpl is no more a rocksdb specific implementation. We can do the rename/removal in follow-up jira.
  • I know there is a plan to refactor datanode part, is there a plan to re-write OMDBStore to follow this new model of DBDefinition?

@elek
Copy link
Member Author

elek commented Apr 17, 2020

Thanks @nandakumar131 the review:

DBDefinition is a generic interface which should not have anything specific to particular db. It's using DBStoreBuilder which is specific to rocksdb.

Good point, I didn't notice it. And definition of the format and the creation logic were mixed in the new DBDefinition interface.

I pushed a new commit (d745282) where all the creation logic is moved to the DBStoreBuilder, the DBDefinition is a simple DB independent interface. (In the future we can also create an interface for DBStoreBuilder and create a RDBStoreBuilder if required.)

SCMMetadataStoreRDBImpl is no more a rocksdb specific implementation. We can do the rename/removal in follow-up jira.

Yes. Agree. Do you prefer rename or removal? I am not sure if it's required or not.

I know there is a plan to refactor datanode part, is there a plan to re-write OMDBStore to follow this new model of DBDefinition?

Yes. I have two motivation:

  • Use only the new DBStore interface everywhere (which would make it easy to add upgrade/versioning support) and fully deprecate the old Metadata interface.

  • Re-create the debug tool (ozone sql If I remember well) which can list/read keys in any DBStore. It requires a unified way to create DB (same DBStore should be created from tools and OM/DN/SCM) and maintain the list of codecs for each table.

Can we use ContainerID instead of Long here?

Not sure. Some of the code uses long instead of CotainerId (eg. updateDeleteTransactionId is based on long, and ContainerInfo is based no long). I can use ContainerId but it requires some temporary ContainerId object creation.

I created a commit in this PR to show how can it work (26c8a58) check the usage of new ContainerId. I am fine with both keeping or reverting it. It's more typesafe with ContainerID and the performance penalty can be very small.

@nandakumar131
Copy link
Contributor

Thanks @elek for updating the patch.

Do you prefer rename or removal? I am not sure if it's required or not.

I'm leaning more towards renaming and using it, as it will give us easy to use API.
If we are going with renaming and using it, it's better to use SCMMetadataStoreRDBImpl in all the places rather than DBStoreBuilder.createDBStore(conf, new SCMDBDefinition()

Instead of this:

DBStore dbStore = DBStoreBuilder.createDBStore(conf, new SCMDBDefinition());
Table<PipelineID, Pipeline> pipelineTable = SCMDBDefinition.PIPELINES.getTable(dbStore);
dbStore.close();

we can use this:

SCMMetadataStore scmMetadataStore = new SCMMetadataStoreRDBImpl(conf);
Table<PipelineID, Pipeline> pipelineTable = scmMetadataStore.getPipelineTable();
scmMetadataStore.close();

Re-create the debug tool (ozone sql If I remember well) which can list/read keys in any DBStore.

+1 on fixing ozone sql tool

@elek
Copy link
Member Author

elek commented Apr 22, 2020

If we are going with renaming and using it, it's better to use SCMMetadataStoreRDBImpl in all the places rather than DBStoreBuilder.createDBStore(conf, new SCMDBDefinition()

I like the idea. Created an issue for this: https://issues.apache.org/jira/browse/HDDS-3479

Marked as newbie but if it won't be picked up by anybody in the next weeks I will do it quickly.

Thanks the review again, will merge it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants