You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/11/03 14:05:04 UTC
[GitHub] [ozone] errose28 opened a new pull request #1544: HDDS 4337. Implement RocksDB options cache for new datanode DB utilities.
errose28 opened a new pull request #1544:
URL: https://github.com/apache/ozone/pull/1544
## What changes were proposed in this pull request
Use a shared static Options object for every column family in every RocksDB instance on a datanode. This results in one configurable cache being used for all RocksDB instances on a datanode. Each RocksDB instance will have its own DBOptions, since this requires the use of RocksDB Statistics objects which are not thread safe and cannot be shared.
The `DBStoreBuilder` class was refactored to make configuration easier. It is now possible to use the same configurable column family options for all column families, and it is easier to reason about which configurations will be applied. It is no longer an error to register a table twice with the builder, the second call will simply override the first, which is usually the expected behavior in setters for builders. There should be no other functionality changes to this class.
## What is the link to the Apache JIRA
HDDS-4337
## How was this patch tested?
`DBStoreBuilder` unit tests were updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r522087101
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
Review comment:
My initial thought was that *metadata* tells what the cache is used for, which seemed more useful than *rocksdb*, which tells how the cache is implemented. But if this config is being changed, knowing that it is for a RocksDB instance might be useful. Maybe we could include both parts? For example, the existing config *ozone.metastore.rocksdb.statistics* contains both pieces of information.
I think putting *container* in the name makes the config sound like it is for one container, whereas just having *datanode* makes it more apparent that this is the size of one shared cache for the whole DN.
These would give a new name like *hdds.datanode.rocksdb.metadata.cache.size* or *hdds.datanode.metadata.store.rocksdb.cache.size* which are informative but fairly long. Since ozone-default.xml has a description, this may not be necessary.
I don't really have a strong opinion either way. If you think *hdds.datanode.container.rocksdb.cache.size* the best name I am fine with that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r521250220
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
+ public static final String
+ HDDS_DATANODE_CACHE_SIZE_DEFAULT = "64MB";
Review comment:
The usual pattern here is to have a pair of variables name like:
PARAMETER_KEY
PARAMETER_DEFAULT
I think it would be good to change this variable to HDDS_DATANODE_METADATA_CACHE_SIZE_DEFAULT.
I see some other parameters in this file don't end in "_KEY", so I guess it is not essential to rename HDDS_DATANODE_METADATA_CACHE_SIZE to "HDDS_DATANODE_METADATA_CACHE_SIZE_KEY".
If we change the parameter name to "hdds.datanode.container.rocksdb.cache.size" based on the above comment, maybe we should also rename the variables to HDDS_DATANODE_CONTAINER_ROCKSDB_CACHE_SIZE ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #1544: HDDS 4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#issuecomment-724052279
If I understand this correctly, in AbstractDatanodeStore.start(), it pulls the default CF options from a static variable where they are setup on first use. This creates a single `LRUCache(CACHE_SIZE)` which is passed into the the DBStoreBuilder for all container RocksDBs. This means they should share the LRU cache.
Its a little hard to follow the logic to ensure this cache is definitely being reused. Could you create a docker cluster, use Freon to create some containers and then check the RocksDB log files to ensure the same cache instance is being used - There is an example of what to look for to prove this in https://issues.apache.org/jira/browse/HDDS-4246.
I also think the 8MB shared cache is probably too small. Could we add a configuration for it, and make the default higher, maybe 64MB for now? What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r522109574
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
Review comment:
Sounds good to me, I'll update the name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r521245605
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
Review comment:
I think a better name for this parameter might be:
"hdds.datanode.container.rocksdb.cache.size"
That more clearly says "this is the cache size for the container rocksDBs". What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r521250220
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
+ public static final String
+ HDDS_DATANODE_CACHE_SIZE_DEFAULT = "64MB";
Review comment:
The usual pattern here is to have a pair of variables name like:
<PARAMETER>_KEY
<PARAMETER>_DEFAULT
I think it would be good to change this variable to HDDS_DATANODE_METADATA_CACHE_SIZE_DEFAULT.
I see some other parameters in this file don't end in "_KEY", so I guess it is not essential to rename HDDS_DATANODE_METADATA_CACHE_SIZE to "HDDS_DATANODE_METADATA_CACHE_SIZE_KEY".
If we change the parameter name to "hdds.datanode.container.rocksdb.cache.size" based on the above comment, maybe we should also rename the variables to HDDS_DATANODE_CONTAINER_ROCKSDB_CACHE_SIZE ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel merged pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
sodonnel merged pull request #1544:
URL: https://github.com/apache/ozone/pull/1544
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on pull request #1544: HDDS 4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
errose28 commented on pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#issuecomment-724124525
Thanks, @sodonnel , I will add a setting for cache size and test this in a docker cluster. Looking at the rocks docs it appears those LRUCache objects can't be accidentally modified by callers, so I could also expose it as public and add a unit test to check that the same cache is used.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r522091456
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
Review comment:
How about `hdds.datanode.metadata.rocksdb.cache.size`? I just dropped `store` from your suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] errose28 commented on a change in pull request #1544: HDDS-4337. Implement RocksDB options cache for new datanode DB utilities.
Posted by GitBox <gi...@apache.org>.
errose28 commented on a change in pull request #1544:
URL: https://github.com/apache/ozone/pull/1544#discussion_r522088395
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -343,6 +343,11 @@
public static final double
HDDS_DATANODE_STORAGE_UTILIZATION_CRITICAL_THRESHOLD_DEFAULT = 0.95;
+ public static final String
+ HDDS_DATANODE_METADATA_CACHE_SIZE = "hdds.datanode.metadata.cache.size";
+ public static final String
+ HDDS_DATANODE_CACHE_SIZE_DEFAULT = "64MB";
Review comment:
Yes, this was a typo. I forgot to update the name of the default variable when I updated the name. I will fix it once we decide on a name for the config.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org