You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-issues@hadoop.apache.org by "Szilard Nemeth (Jira)" <ji...@apache.org> on 2021/07/29 11:25:00 UTC
[jira] [Comment Edited] (YARN-10838) Implement an optimised version
of Configuration getPropsWithPrefix
[ https://issues.apache.org/jira/browse/YARN-10838?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17389789#comment-17389789 ]
Szilard Nemeth edited comment on YARN-10838 at 7/29/21, 11:24 AM:
------------------------------------------------------------------
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):
{code}
*split\(DELIMITER.*"
{code}
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?
was (Author: snemeth):
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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org