HDDS-3755. Storage-class support for Ozone#1208
Conversation
I think the easiest approach can be to modify only the |
…n existing bucket. (apache#1251)
|
@elek Do you think we should use StorageClass class to replace the String for StorageClass type within a Service, we use string to transfer storageClass only in client to OM. Do you think this will make sense? |
|
Thanks for the help from @maobaolong , it's ready to review. The key points of the patch:
Instead of: you can use:
Remaining logic is the same.
|
|
The POC patch looks great, we can flexibly adjust the replica number/ storage type for the storage. This is very similar to the HDFS storage policy design, some high level review comments from me:
|
|
Thanks the questions @linyiqun
I am not sure if I understood the question well, but let me try to answer: I think this change is backward compatible as RPC (client!) interface accept both requests: the old one (which contains factor/type and no storageClass) and the new one (which contains storageClass and no factor/type).
Dynamic switch is hard, because storageClass is a property of the containers. When you modify the storageClass of a bucket/key the related data should me moved out from one container to a new one. Amazon doesn't enable the modification of storageClass and I suggested the same. However, we can support to change the "definition" of the storage class. Currently, it's hard coded, but it's easy to be modified to a dynamic model. When you modify the closed container replication number of a storage class (let's say STANDARD close is THREE today but you would like to make it TWO) ReplicationManager can easily pick up the changes.
Today the transition between OPEN and CLOSED state is hard coded. Later we can make it more dynamic (for example disable the transition for some specific storageClass) but we are not yet there. On the other hand: the configuration can be changed. Today STANDARD = Ratis/THREE -> Closed/THREE but it can be changed to be configurable (for example support Ratis/THREE -> Ratis/TWO) I am not sure, if this was the question, let me know if not.
Definitely, this should be one of the next steps. |
|
Thanks @elek for the detailed comments.
Dynamic model seems a good way, so this storage class will bee a user-defined storage class, right? And then, we should persist storage class info (factor/type) into db and admin user can update the storage class info via CLI command way. But anyway, current POC overall looks good to me. |
As a first step I would be happy to make it configurable (instead of having the config build time) and admin can update the config and restart the cluster. But yes, later it can be more and more dynamic (like storing it and providing CLI) This patch is mainly about the framework. If we have storage class as an abstraction level we can add more and more configuration options: for example the storage format on the datanode, erasure coding parameters. Or we can create an experimental, different write path without changing the existing code. Storage-class abstraction can help to separate the configuration from the user interface. User can choose from understandable storage class names and admin can (re)configure the details in the background. |
|
I did the following test, it proved that storageClass can run as expected. |
|
@arp7 @umamaheswararao do you have any more comments / questions about the concept and / or about the patch? |
|
Closing it temporary. We need #1419 to be accepted. When it's accepted, this branch can be mosted in smaller chunks (proto changes + SCM changes + ...) |
Created together with @maobaolong (thanks the help)
What changes were proposed in this pull request?
This is the initial draft for storage-class support. It introduces the storage-class and uses it instead of replication factor / type.
Legacy clients are supported with converting back to factor / type to storage-class.
Storage classes are hard-coded in the java code (can be changed later to be configurable).
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3755
How was this patch tested?
With CI tests.