[jira] [Commented] (YARN-11415) Refactor TestConfigurationFieldsBase and the connected test classes
[
https://issues.apache.org/jira/browse/YARN-11415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17696896#comment-17696896
]
Szilard Nemeth commented on YARN-11415:
---
Based on our offline discussion with [~bteke] , I'm closing this.
> Refactor TestConfigurationFieldsBase and the connected test classes
> ---
>
> Key: YARN-11415
> URL: https://issues.apache.org/jira/browse/YARN-11415
> Project: Hadoop YARN
> Issue Type: Improvement
>Reporter: Benjamin Teke
>Assignee: Szilard Nemeth
>Priority: Major
> Labels: pull-request-available
>
> YARN-11413 pointed out a strange way of how the configuration tests are
> executed. The first problem is that there is a
> [Pattern|https://github.com/apache/hadoop/blob/570b503e3e7e7adf5b0a8fabca76003298216543/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfigurationFieldsBase.java#L197],
> that matches only numbers, letters, dots, hyphens and underscores, but not
> %, which is used in string replacements (i.e
> {{yarn.nodemanager.aux-services.%s.classpath}} ), so essentially every
> property that's present in any configuration object and doesn't match this
> pattern is silently skipped, and documenting it will result in invalid test
> failures, ergo the test encourages introducing props and not documenting
> them. The pattern should be fixed in YARN-11413 for %s, but it's necessity
> could be checked.
> Another issue with this is that it works in a semi-opposite way of what it's
> supposed to do. To ensure all of the configuration entries are documented it
> should iterate through all of the configuration fields and check if those
> have matching xyz-default.xml entries, but currently it just reports the
> entries that are present in the xyz-default.xml and missing in the matching
> configuration file. Since this test checks all the configuration objects this
> might need some other follow-ups to document the missing properties from
> other components if there are any.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-11415) Refactor TestConfigurationFieldsBase and the connected test classes
[
https://issues.apache.org/jira/browse/YARN-11415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17696895#comment-17696895
]
ASF GitHub Bot commented on YARN-11415:
---
szilard-nemeth closed pull request #5454: YARN-11415: Proof --> DO NOT COMMIT
YET
URL: https://github.com/apache/hadoop/pull/5454
> Refactor TestConfigurationFieldsBase and the connected test classes
> ---
>
> Key: YARN-11415
> URL: https://issues.apache.org/jira/browse/YARN-11415
> Project: Hadoop YARN
> Issue Type: Improvement
>Reporter: Benjamin Teke
>Assignee: Szilard Nemeth
>Priority: Major
> Labels: pull-request-available
>
> YARN-11413 pointed out a strange way of how the configuration tests are
> executed. The first problem is that there is a
> [Pattern|https://github.com/apache/hadoop/blob/570b503e3e7e7adf5b0a8fabca76003298216543/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfigurationFieldsBase.java#L197],
> that matches only numbers, letters, dots, hyphens and underscores, but not
> %, which is used in string replacements (i.e
> {{yarn.nodemanager.aux-services.%s.classpath}} ), so essentially every
> property that's present in any configuration object and doesn't match this
> pattern is silently skipped, and documenting it will result in invalid test
> failures, ergo the test encourages introducing props and not documenting
> them. The pattern should be fixed in YARN-11413 for %s, but it's necessity
> could be checked.
> Another issue with this is that it works in a semi-opposite way of what it's
> supposed to do. To ensure all of the configuration entries are documented it
> should iterate through all of the configuration fields and check if those
> have matching xyz-default.xml entries, but currently it just reports the
> entries that are present in the xyz-default.xml and missing in the matching
> configuration file. Since this test checks all the configuration objects this
> might need some other follow-ups to document the missing properties from
> other components if there are any.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-11415) Refactor TestConfigurationFieldsBase and the connected test classes
[
https://issues.apache.org/jira/browse/YARN-11415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17696620#comment-17696620
]
ASF GitHub Bot commented on YARN-11415:
---
hadoop-yetus commented on PR #5454:
URL: https://github.com/apache/hadoop/pull/5454#issuecomment-1455196102
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 29s | | Docker mode activated. |
_ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files
found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available.
|
| +0 :ok: | xmllint | 0m 0s | | xmllint was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain
any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to
include 1 new or modified test files. |
_ trunk Compile Tests _ |
| +0 :ok: | mvndep | 15m 14s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 29m 3s | | trunk passed |
| +1 :green_heart: | compile | 25m 39s | | trunk passed with JDK
Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | compile | 21m 51s | | trunk passed with JDK
Private Build-1.8.0_362-8u362-ga-0ubuntu1~20.04.1-b09 |
| +1 :green_heart: | checkstyle | 4m 3s | | trunk passed |
| +1 :green_heart: | mvnsite | 3m 33s | | trunk passed |
| +1 :green_heart: | javadoc | 2m 47s | | trunk passed with JDK
Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 2m 15s | | trunk passed with JDK
Private Build-1.8.0_362-8u362-ga-0ubuntu1~20.04.1-b09 |
| +1 :green_heart: | spotbugs | 6m 39s | | trunk passed |
| +1 :green_heart: | shadedclient | 24m 12s | | branch has no errors
when building and testing our client artifacts. |
_ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 23s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 17s | | the patch passed |
| +1 :green_heart: | compile | 24m 32s | | the patch passed with JDK
Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javac | 24m 32s | | the patch passed |
| +1 :green_heart: | compile | 21m 38s | | the patch passed with JDK
Private Build-1.8.0_362-8u362-ga-0ubuntu1~20.04.1-b09 |
| +1 :green_heart: | javac | 21m 38s | | the patch passed |
| -1 :x: | blanks | 0m 0s |
[/blanks-eol.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5454/1/artifact/out/blanks-eol.txt)
| The patch has 1 line(s) that end in blanks. Use git apply --whitespace=fix
<>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | checkstyle | 3m 51s | | root: The patch generated
0 new + 209 unchanged - 2 fixed = 209 total (was 211) |
| +1 :green_heart: | mvnsite | 3m 25s | | the patch passed |
| +1 :green_heart: | javadoc | 2m 36s | | the patch passed with JDK
Ubuntu-11.0.18+10-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 2m 15s | | the patch passed with JDK
Private Build-1.8.0_362-8u362-ga-0ubuntu1~20.04.1-b09 |
| +1 :green_heart: | spotbugs | 6m 55s | | the patch passed |
| +1 :green_heart: | shadedclient | 24m 25s | | patch has no errors
when building and testing our client artifacts. |
_ Other Tests _ |
| +1 :green_heart: | unit | 18m 15s | | hadoop-common in the patch
passed. |
| -1 :x: | unit | 1m 8s |
[/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5454/1/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt)
| hadoop-yarn-api in the patch passed. |
| +1 :green_heart: | unit | 5m 26s | | hadoop-yarn-common in the patch
passed. |
| +1 :green_heart: | asflicense | 0m 54s | | The patch does not
generate ASF License warnings. |
| | | 255m 50s | | |
| Reason | Tests |
|---:|:--|
| Failed junit tests | hadoop.yarn.conf.TestYarnConfigurationFields |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.42 ServerAPI=1.42 base:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5454/1/artifact/out/Dockerfile
|
| GITHUB PR | https://github.com/apache/hadoop/pull/5454 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall
mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint |
| uname | Linux 8249d3db673d 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24
18:16:04 UTC 2022 x86_64
[jira] [Commented] (YARN-11415) Refactor TestConfigurationFieldsBase and the connected test classes
[
https://issues.apache.org/jira/browse/YARN-11415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17696584#comment-17696584
]
Szilard Nemeth commented on YARN-11415:
---
Hi [~bteke],
I just briefly checked this.
{quote}
Another issue with this is that it works in a semi-opposite way of what it's
supposed to do. To ensure all of the configuration entries are documented it
should iterate through all of the configuration fields and check if those have
matching xyz-default.xml entries, but currently it just reports the entries
that are present in the xyz-default.xml and missing in the matching
configuration file. Since this test checks all the configuration objects this
might need some other follow-ups to document the missing properties from other
components if there are any.
{quote}
I think what you stated here is true for this method:
TestCommonConfigurationFields#testCompareXmlAgainstConfigurationClass.
This method compares the properties that are in yarn-default.xml, but not in
the Configuration class.
With my first commit I added a dummy property to yarn-default.xml without
adding it to the YarnConfiguration class.
The property called "yarn.nodemanager.missingpropinclass", but the name doesn't
really matter.
Test result: Failure in
TestCommonConfigurationFields#testCompareXmlAgainstConfigurationClass:
{code}
java.lang.AssertionError: yarn-default.xml has 1 properties missing in class
org.apache.hadoop.yarn.conf.YarnConfiguration Entries:
yarn.nodemanager.missingpropinclass
Expected :0
Actual :1
{code}
However, we also have
TestCommonConfigurationFields#testCompareConfigurationClassAgainstXml which
compares the properties that are in the YarnConfiguration class, but not
defined in yarn-default.xml.
So with my second commit, I added this to YarnConfiguration:
{code}
public static final String MISSING_PROP_IN_YARN_DEF =
"yarn.missingprop.in.yarndefault";
{code}
without touching the yarn-default.xml so the new config was not documented.
As I expected, the testcase
TestConfigurationFieldsBase#testCompareConfigurationClassAgainstXml failed
with:
{code}
java.lang.AssertionError: class org.apache.hadoop.yarn.conf.YarnConfiguration
has 1 variables missing in yarn-default.xml Entries:
yarn.missingprop.in.yarndefault
Expected :0
Actual :1
{code}
I think so far so good, this is the expected behavior.
The main issue before YARN-11413 was:
1.
org.apache.hadoop.conf.TestConfigurationFieldsBase#setupTestConfigurationFields
is called as a "\@Before" method.
2.
org.apache.hadoop.conf.TestConfigurationFieldsBase#extractMemberVariablesFromConfigurationFields
is called.
3. All of the fields of the class is checked here with certain restrictions.
Among these are that it should be a public static final String, and it should
match a Pattern.
If the pattern is not matched, the field won't be added to the "known fields"
for sure.
4. So with my third commit, I just removed the percent sign (basically a revert
of YARN-11415) to see what happens.
TestConfigurationFieldsBase#testCompareXmlAgainstConfigurationClass fails,
which is a false positive now.
Here's the assertion message:
{code}
java.lang.AssertionError: yarn-default.xml has 2 properties missing in class
org.apache.hadoop.yarn.conf.YarnConfiguration Entries:
yarn.nodemanager.aux-services.%s.classpath
yarn.nodemanager.aux-services.%s.system-classes
Expected :0
Actual :2
{code}
This is indeed wrong, as per your description.
However, I don't see how this could be fixed in a clean way.
Here, the configuration fields of YarnConfiguration were not recognized:
yarn.nodemanager.aux-services.%s.classpath
yarn.nodemanager.aux-services.%s.system-classes.
What can we do then?
The whole point of matching the values of the String fields is to differentiate
config keys from other strings like:
{code}
public static final String NVIDIA_DOCKER_V1 = "nvidia-docker-v1";
{code}
I cannot see a better way than what we are currently doing with the regex.
I think the regex pattern should be continuously maintained and occassions like
unmatched real config keys like "yarn.nodemanager.aux-services.%s.classpath"
(because of the regex didn't contain the percentage sign) could be quite rare.
[~bteke]
Do you have anything in mind about a clean fix for this?
Anyway, I will report another jira where I will list my suggested improvements
of the test and its base class: TestConfigurationFieldsBase.
> Refactor TestConfigurationFieldsBase and the connected test classes
> ---
>
> Key: YARN-11415
> URL: https://issues.apache.org/jira/browse/YARN-11415
> Project: Hadoop YARN
> Issue Type: Improvement
>Reporter: Benjamin Teke
>Assignee: Szilard Nemeth
>Priority: Major
> Labels:
[jira] [Commented] (YARN-11415) Refactor TestConfigurationFieldsBase and the connected test classes
[
https://issues.apache.org/jira/browse/YARN-11415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17696583#comment-17696583
]
ASF GitHub Bot commented on YARN-11415:
---
szilard-nemeth opened a new pull request, #5454:
URL: https://github.com/apache/hadoop/pull/5454
### Description of PR
### How was this patch tested?
### For code changes:
- [ ] Does the title or this PR starts with the corresponding JIRA issue id
(e.g. 'HADOOP-17799. Your PR title ...')?
- [ ] Object storage: have the integration tests been executed and the
endpoint declared according to the connector-specific documentation?
- [ ] If adding new dependencies to the code, are these dependencies
licensed in a way that is compatible for inclusion under [ASF
2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE`, `LICENSE-binary`,
`NOTICE-binary` files?
> Refactor TestConfigurationFieldsBase and the connected test classes
> ---
>
> Key: YARN-11415
> URL: https://issues.apache.org/jira/browse/YARN-11415
> Project: Hadoop YARN
> Issue Type: Improvement
>Reporter: Benjamin Teke
>Assignee: Szilard Nemeth
>Priority: Major
>
> YARN-11413 pointed out a strange way of how the configuration tests are
> executed. The first problem is that there is a
> [Pattern|https://github.com/apache/hadoop/blob/570b503e3e7e7adf5b0a8fabca76003298216543/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfigurationFieldsBase.java#L197],
> that matches only numbers, letters, dots, hyphens and underscores, but not
> %, which is used in string replacements (i.e
> {{yarn.nodemanager.aux-services.%s.classpath}} ), so essentially every
> property that's present in any configuration object and doesn't match this
> pattern is silently skipped, and documenting it will result in invalid test
> failures, ergo the test encourages introducing props and not documenting
> them. The pattern should be fixed in YARN-11413 for %s, but it's necessity
> could be checked.
> Another issue with this is that it works in a semi-opposite way of what it's
> supposed to do. To ensure all of the configuration entries are documented it
> should iterate through all of the configuration fields and check if those
> have matching xyz-default.xml entries, but currently it just reports the
> entries that are present in the xyz-default.xml and missing in the matching
> configuration file. Since this test checks all the configuration objects this
> might need some other follow-ups to document the missing properties from
> other components if there are any.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
