[
https://issues.apache.org/jira/browse/YARN-10838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17389789#comment-17389789
]
Szilard Nemeth commented on YARN-10838:
---------------------------------------
Thanks [~gandras] for working on this.
Nicely done patch and the code is clean in general, especially in the newly
added ConfigurationProperties class.
1. Nit: In the javadoc of ConfigurationProperties: The sentence ending with
'part delimited by ".".' should have a line break before the next sentence.
2. Nit: In the javadoc of ConfigurationProperties: I think it would be useful
to mention that this "storage" is a Trie data structure:
https://en.wikipedia.org/wiki/Trie#:~:text=In%20computer%20science%2C%20a%20trie,key%2C%20but%20by%20individual%20characters.
3. Nit: There are two occasions where a prefix string is split by the DELIMITER
(dot).
Just look for usages (regex search): ".*split\(DELIMITER.*"
So I think you could introduce a method that does this, named like
"splitPrefixByDelimiter".
Moreover, both the usages convert the String array to a List of Strings, so the
method could combine the split + convert to list operations.
4. In ConfigurationProperties#storePropertiesInPrefixNodes, there is a
condition that checks if propertyKeyParts.length > 0. Maybe we could add a
warning log if the length is not greater than zero. Speaking of which, I don't
think there are too many properties without a dot in their key, so this could
be a safe operation to do.
5. The javadoc of ConfigurationProperties.PrefixNode is not talking too much
about the values and children fields of a particular PrefixNode. Could you add
some words about those fields as well?
6. Nit: In ConfigurationProperties, I think the method named
"recursivelyCollectProperties" should rather be "collectPropertiesRecursively".
7. Nit: In TestConfigurationProperties, I don't see a clear reason why to
include the "test" prefix for all nodes. I would just use numbers for better
readability, and the first property ("test", currently) could be simply "root".
8. In TestConfigurationProperties, I don't see any testcase that would assert
for the values themselves (TEST_VALUE_1, TEST_VALUE_2, ...). Could you please
add testcases to check those values?
9. Last but not least: I can't see any caller of
CapacitySchedulerConfiguration#getConfigurationProperties, so basically you
added new code but the production code is not utilizing it. Is this
intentional?
> Implement an optimised version of Configuration getPropsWithPrefix
> ------------------------------------------------------------------
>
> Key: YARN-10838
> URL: https://issues.apache.org/jira/browse/YARN-10838
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Andras Gyori
> Assignee: Andras Gyori
> Priority: Major
> Attachments: YARN-10838.001.patch, YARN-10838.002.patch,
> YARN-10838.003.patch, YARN-10838.004.patch
>
>
> AutoCreatedQueueTemplate also has multiple call to
> Configuration#getPropsWithPrefix. It must be eliminated in order to improve
> the performance on reinitialisation.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
