[GitHub] [hadoop] liuml07 commented on a change in pull request #2241: HADOOP-17222. Create socket address combined with cache

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org