liuml07 commented on a change in pull request #2241:
URL: https://github.com/apache/hadoop/pull/2241#discussion_r478114810
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsClientConfigKeys.java
##########
@@ -414,6 +415,11 @@
String PREFETCH_SIZE_KEY = PREFIX + "prefetch.size";
+ String URI_CACHE_KEY = PREFIX + "uri.cache.enable";
+ boolean URI_CACHE_DEFAULT = false;
+ String URI_CACHE_EXPIRE_MS_KEY = PREFIX + "uri.cache.expire.ms";
Review comment:
I think it's better not to have this config as I suggested in the JIRA.
It's for two reasons:
1. This seems not critical parameter and Hadoop already has too many
configurations. I think the default value 12hours is good enough. It's not
meant to be tuned by per client since the cache is static and shared. So
removing this config seems preferred to me.
2. The URI cache is initialized only once. So passing the `uriCacheExpireMs`
parameter overtime does not really carry enough information. And it may be
confusing if people mistakenly interpret this as a per-URI setting when they
call `NetUtils.createSocketAddr()` which is not.
Thoughts?
##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
##########
@@ -4185,6 +4185,16 @@
</description>
</property>
+<property>
+ <name>dfs.client.read.uri.cache.enable</name>
+ <value>false</value>
+ <description>
+ If true, dfs client will use cache when creating URI based on host:port
+ to reduce the frequency of URI object creation.
+ Default is false.
Review comment:
nit: let's remove this sentence. The default value is clear in 5 lines
above. Otherwise if we update the default value in future, we may miss this one
which will cause confusion.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsClientConfigKeys.java
##########
@@ -414,6 +414,9 @@
String PREFETCH_SIZE_KEY = PREFIX + "prefetch.size";
+ String URI_CACHE_KEY = PREFIX + "uri.cache.enable";
Review comment:
nit: name this `...uri.cache.enabled` by replacing `enable` with
`enabled`?
Also change other variables related to `uriCacheEnable`
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
