[GitHub] [gobblin] sv2000 commented on a change in pull request #3335: [GOBBLIN-1492] Optimize flowspec keys on configToProperties

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

sv2000 commented on a change in pull request #3335:
URL: https://github.com/apache/gobblin/pull/3335#discussion_r678622001



##########
File path: gobblin-utility/src/main/java/org/apache/gobblin/util/ConfigUtils.java
##########
@@ -116,7 +116,8 @@ public static Properties configToProperties(Config config, Optional<String> pref
       Config resolvedConfig = config.resolve();
       for (Map.Entry<String, ConfigValue> entry : resolvedConfig.entrySet()) {
         if (!prefix.isPresent() || entry.getKey().startsWith(prefix.get())) {
-          String propKey = desanitizeKey(entry.getKey());
+          // Intern the string so that constant keys are not duplicated to save memory
+          String propKey = desanitizeKey(entry.getKey()).intern();

Review comment:
       @Will-Lo This method is invoked in a number of places and not limited to Gobblin service. One concern is that the number of keys may not be as small as you imagine. In production settings, Gobblin jobs could load system configs set by AZ/Hadoop or even the host where a job/container is running. These can be non-trivial in number. 60k may be sufficient. But what happens if the number of config keys exceed this value? Will the JVM just crash?  




-- 
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.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org