[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17278425#comment-17278425 ] Ahmed Hussein commented on YARN-10585: -- The process to deal with those fixes is not defined in the community. Therefore, it is done by personal styling and preference. My point regarding the difference between reverting Vs filing-new-jira: * Yetus analyses the code based on the diff. This means that splitting the PR into two phases implies that the UTs and the code analysis have not been done on the whole changes together. These are couple of 2 sample examples for such cases: ** Take YARN-10352 which were committed with two findbugs errors. Both errors were lost because the report expired. The followup Jira YARN-10611 that was supposed to fix an import, shows only one findbugs report. ** Another example: if the follow-up Jira does not touch UT files, then Yetus won't trigger the tests cases. If the follow-up fixes break the unit tests, Yetus won't detect that leading to the merge of the broken code. * While I agree that findbugs/checkstyles reports have a lot of false-positives, they occasionally point out to bugs. This was the case with YARN-10352 which breaks the Hadoop dependencies. * In the last couple of weeks, there were at least 3 code merges with Yetus errors, with the first one being breaking the dependencies of Guava: 1) YARN-10352 - YARN-10611 , 2) YARN-10574 -YARN-10506 , 3) YARN-10585- YARN-10612 . > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Fix For: 3.4.0 > > Attachments: YARN-10585.001.patch, YARN-10585.002.patch, > YARN-10585.003.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17278381#comment-17278381 ] Ahmed Hussein commented on YARN-10585: -- Thank you [~shuzirra] and [~snemeth] for the clarification. [~snemeth] Sorry that I sounded negative and I did not word my comment the best way. I did not mean to comment on the quality of the work. What I meant was that the credibility of the process will diminish when it becomes a habit. I am confident you have verified the patch and the UTs. I believe you have a good point to keep this Jira as resolved while fixing the issue in YARN-10612. Apologies for reopening this Jira. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Fix For: 3.4.0 > > Attachments: YARN-10585.001.patch, YARN-10585.002.patch, > YARN-10585.003.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17278360#comment-17278360
]
Gergely Pollak commented on YARN-10585:
---
[~ahussein] thank you for your suggestions.
{code:java}
For future code mergse and commits, please make sure that the patch/PR does not
generate Yetus errors before merging.
{code}
While I'm really sorry for the oversight, mistakes unfortunately do happen, as
soon we realized it we opened an other Jira to fix it. Please let's not argue
about what are the "proper way to fix things" but rather let's focus on
actually fixing the thing.
{code:java}
It is not scalable to have several Jiras filed just to fix checkstyle, and
findbugs.
{code}
It has nothing to do with scalability, also we are not planning to make it a
habit, but I think it's more important to get this resolved than to worry about
one extra JIRA.
> Create a class which can convert from legacy mapping rule format to the new
> JSON format
> ---
>
> Key: YARN-10585
> URL: https://issues.apache.org/jira/browse/YARN-10585
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Gergely Pollak
>Assignee: Gergely Pollak
>Priority: Major
> Fix For: 3.4.0
>
> Attachments: YARN-10585.001.patch, YARN-10585.002.patch,
> YARN-10585.003.patch
>
>
> To make transition easier we need to create tooling to support the migration
> effort. The first step is to create a class which can migrate from legacy to
> the new JSON format.
--
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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17278337#comment-17278337 ] Szilard Nemeth commented on YARN-10585: --- Hi [~ahussein], My thoughts: 1. Apologies for merging this one with the Findbugs issue. I have been a committer since middle of 2019 and have been paying attention and have been striving for the best code quality and Yetus results, making sure the code meets the code quality standards we're expecting at Hadoop. This one is an exceptional case that simply fell through the cracks. 2. About the UT failures: They are completely unrelated - org.apache.hadoop.yarn.server.resourcemanager.TestRMRestart.testRMRestartOnMissingAttempts[FAIR]: This is Fair scheduler related and the patch is not - org.apache.hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer.testRMRestartWithExpiredToken: This is a well known flakey. 3. I can see that [~shuzirra] already reported YARN-10612 and you also left a comment there. I still don't understand how reopening this jira is a better approach than fixing it in a follow-up. We will have one more commit on top of trunk nevertheless, as I would not revert this commit for the sake of a single findbugs warning. You mentioned amending on the other jira. How did you mean that? I never amended any commit as it modified git history and this is to be avoided on a repository that is used by many many people. 4. About scalability: I generally agree with your comment but as said in bullet point 1, this is an excecptional situation. I have 200+ commits and I can't recall a case where I committed findbugs issues. So it's a bit of an overstatement that this will cause a flood of commits. 5. Credibility: I can agree that we need to strive for findbugs error free commits. However, I have carefully reviewed the unit tests Gergo introduced and the coverage was more than enough. Such an NPE would have surfaced during the UT execution as well. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Fix For: 3.4.0 > > Attachments: YARN-10585.001.patch, YARN-10585.002.patch, > YARN-10585.003.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17278258#comment-17278258 ] Ahmed Hussein commented on YARN-10585: -- Thanks [~shuzirra] and [~snemeth] for the contribution. I am reopening this jira as it was merged with Yetus failures. For future code mergse and commits, please make sure that the patch/PR does not generate Yetus errors before merging. It is not scalable to have several Jiras filed just to fix checkstyle, and findbugs. In addition to the fact that this raises doubts on the patch credibility overall; eventually, this causes a flood of commits and difficulty reverting commits leading to unstable code repository. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Fix For: 3.4.0 > > Attachments: YARN-10585.001.patch, YARN-10585.002.patch, > YARN-10585.003.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17272281#comment-17272281
]
Hadoop QA commented on YARN-10585:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 25m
49s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} || ||
| {color:green}+1{color} | {color:green} dupname {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} No case conflicting files
found. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch does not contain any
@author tags. {color} |
| {color:green}+1{color} | {color:green} {color} | {color:green} 0m 0s{color}
| {color:green}test4tests{color} | {color:green} The patch appears to include 3
new or modified test files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 22m
59s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
3s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.20.04 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
52s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~20.04-b01 {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
46s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
19s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
21m 25s{color} | {color:green}{color} | {color:green} branch has no errors when
building and testing our client artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
59s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.20.04 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m
4s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~20.04-b01 {color} |
| {color:blue}0{color} | {color:blue} spotbugs {color} | {color:blue} 2m
43s{color} | {color:blue}{color} | {color:blue} Used deprecated FindBugs
config; considering switching to SpotBugs. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
39s{color} | {color:green}{color} | {color:green} trunk passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
12s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
24s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.20.04 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m
24s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
16s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Private Build-1.8.0_275-8u275-b01-0ubuntu1~20.04-b01 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m
16s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
0m 48s{color} |
{color:orange}https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/547/artifact/out/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt{color}
| {color:orange}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:
The patch generated 2 new + 5 unchanged - 0 fixed = 7 total (was 5) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
47s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch has no whitespace
issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
15m 5s{color} | {color:green}{color} | {color:green} patch has no errors when
building and testing our client artifacts. {color} |
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17272274#comment-17272274 ] Szilard Nemeth commented on YARN-10585: --- Thanks [~shuzirra] for this, good job! Latest patch LGTM, committed to trunk. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Fix For: 3.4.0 > > Attachments: YARN-10585.001.patch, YARN-10585.002.patch, > YARN-10585.003.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17272089#comment-17272089
]
Hadoop QA commented on YARN-10585:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
0s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
| {color:red}-1{color} | {color:red} docker {color} | {color:red} 0m
5s{color} | {color:red}{color} | {color:red} Docker failed to build
yetus/hadoop:b441ca86995. {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | YARN-10585 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/13019410/YARN-10585.003.patch |
| Console output |
https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/545/console |
| versions | git=2.17.1 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
> Create a class which can convert from legacy mapping rule format to the new
> JSON format
> ---
>
> Key: YARN-10585
> URL: https://issues.apache.org/jira/browse/YARN-10585
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Gergely Pollak
>Assignee: Gergely Pollak
>Priority: Major
> Attachments: YARN-10585.001.patch, YARN-10585.002.patch,
> YARN-10585.003.patch
>
>
> To make transition easier we need to create tooling to support the migration
> effort. The first step is to create a class which can migrate from legacy to
> the new JSON format.
--
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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17272072#comment-17272072 ] Gergely Pollak commented on YARN-10585: --- Latest patch fixed [~gandras] "userGroupMappingRules and applicationNameMappingRules might be initialised as empty sets" concern, now the null check is centralised and moved to the setters. Also fixed ASF warnings, and added a testcase for the "u:%user:root.%user.QUEUE" case. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Attachments: YARN-10585.001.patch, YARN-10585.002.patch, > YARN-10585.003.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17272073#comment-17272073
]
Hadoop QA commented on YARN-10585:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
0s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
| {color:red}-1{color} | {color:red} docker {color} | {color:red} 0m
6s{color} | {color:red}{color} | {color:red} Docker failed to build
yetus/hadoop:b441ca86995. {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | YARN-10585 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/13019410/YARN-10585.003.patch |
| Console output |
https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/544/console |
| versions | git=2.17.1 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
> Create a class which can convert from legacy mapping rule format to the new
> JSON format
> ---
>
> Key: YARN-10585
> URL: https://issues.apache.org/jira/browse/YARN-10585
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Gergely Pollak
>Assignee: Gergely Pollak
>Priority: Major
> Attachments: YARN-10585.001.patch, YARN-10585.002.patch,
> YARN-10585.003.patch
>
>
> To make transition easier we need to create tooling to support the migration
> effort. The first step is to create a class which can migrate from legacy to
> the new JSON format.
--
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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17271405#comment-17271405 ] zhuqi commented on YARN-10585: -- Thank [~shuzirra] for working on this. The latest patch LGTM. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Attachments: YARN-10585.001.patch, YARN-10585.002.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17271371#comment-17271371 ] Benjamin Teke commented on YARN-10585: -- Thank [~shuzirra] for working on this. Based on your answers to [~gandras]'s comments the patch looks good to me, so +1 (non-binding) from my side. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Attachments: YARN-10585.001.patch, YARN-10585.002.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17271297#comment-17271297
]
Gergely Pollak commented on YARN-10585:
---
[~gandras] thank you for the review and your comments, let me address your
concerns.
* Nit: in splitRule#line:213 the classic for loop could be replaced by the
enhanced loop/stream, which is nicer to readI
It is VERY subjective what is nice / easy to read, but in this case it's quite
hard to contradict this statement. Literally ANY developer who ever wrote a
loop, will be able to read a simple for loop, while to understand streams they
need some deeper java knowledge, so I think it points out very well, why is the
for loop easier (this nicer) to read.
But let's get technical a bit, because the overuse of the java streams is a pet
peeve of mine. Java streams with lambdas will create an unnecessary (well for
streams they ARE necessary) anonymous class in the background, and wrap the
actual loop core with multiple method calls, which inherently put extra strain
on the stack, gc and on the performance, for a very subjective little gain.
{code:java}
java.lang.ArithmeticException: / by zero
at fiddle.main(fiddle.java:7)
java.lang.ArithmeticException: / by zero
at fiddle.lambda$main$0(fiddle.java:14)
at
java.util.stream.Streams$RangeIntSpliterator.forEachRemaining(Streams.java:110)
at java.util.stream.IntPipeline$Head.forEach(IntPipeline.java:581)
at fiddle.main(fiddle.java:14){code}
We can see the extra method calls and the created class as well.
This has an effect on performance as well:
{code:java}
int sum = 0;
for (int i = 0; i < 10; i++) {
sum += i;
}
{code}
Runs on my computer about 330ms
{code:java}
IntStream.range(0, 10).sum();{code}
This has a bit of a range between 380-420 ms, but even in best case it is still
a 20% performance degradation.
So all in all, I prefer to avoid streams, when they are not strictly needed,
because they are not to replace regular loops but to add some functional
programming support to java. They have their place in the language, for example
when one wants to use parallel streams they can really improve the performance,
with much less hassle about thread handling and concurrency.
* Nit: in creatUserMappingRule, you replace the match argument with its json
equivalent (*). Overwriting arguments are generally not a good idea, because it
could produce hard-to-find errors, also made the debugging somewhat harder.
As a general rule it's true, however I'm converting the argument in place,
before using it, it is not a repurpose of the variable, but I'm making sure,
the function receives the input it will properly process, in this case I don't
really find it problematic, since the goal here is to "hide" the incorrect
value from the rest of the method, hence the overwrite.
* I am not sure about this, but is the following format accepted in the legacy
format:
Yeah, it is a complex question. It is NOT supported by the legacy engine, but
it works in the new engine with legacy mode... however I've took a look at it,
and actually the converter DOES support this conversion, it converts it to a
customRule. I don't want to add any validation to the input, besides it's
syntax, because the tool will do a best effort conversion for unsupported
cases. It should be able to convert EVERY use case supported by the legacy
engine, and all cases supported by the new engine in legacy format, however we
don't encourage to use the new features in the legacy format. So if someone
still uses them we might be able to convert those rules as well (probably they
will become custom rules) so I don't want to reject those just because they are
not officially supported.
* userGroupMappingRules and applicationNameMappingRules might be initialised
as empty sets. This would eliminate the null checks in convert loops, but the
empty cases could still be checked in the beginning.
Since I expose the collection setter method then I'd have to move the null
check there, so we don't really save null checks, but at least we don't create
unnecessary objects. However I like the idea to have a centralised null check,
so I'll see the other feedbacks, and if I'll make a new patch set I'll consider
this suggestion.
> Create a class which can convert from legacy mapping rule format to the new
> JSON format
> ---
>
> Key: YARN-10585
> URL: https://issues.apache.org/jira/browse/YARN-10585
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Gergely Pollak
>Assignee: Gergely Pollak
>Priority: Major
> Attachments: YARN-10585.001.patch, YARN-10585.002.patch
>
>
> To make t
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17271253#comment-17271253
]
Andras Gyori commented on YARN-10585:
-
Thank you [~shuzirra] for the patch! I have analysed the patch and here is my
findings:
* Nit: in splitRule#line:213 the classic for loop could be replaced by the
enhanced loop/stream, which is nicer to read
* Nit: in creatUserMappingRule, you replace the match argument with its json
equivalent (*). Overwriting arguments are generally not a good idea, because it
could produce hard-to-find errors, also made the debugging somewhat harder.
* I am not sure about this, but is the following format accepted in the legacy
format:
{noformat}
u:%user:root.%user.QUEUE{noformat}
Namely, if the %user matcher is not the last item of the rule. This would
complicate things, and I think the current code could not handle this. If it is
not supported, can you make an additional error checking on this case?
* userGroupMappingRules and applicationNameMappingRules might be initialised
as empty sets. This would eliminate the null checks in convert loops, but the
empty cases could still be checked in the beginning.
Since you have written a lot of unit test cases, I think the logic itself is
correct.
> Create a class which can convert from legacy mapping rule format to the new
> JSON format
> ---
>
> Key: YARN-10585
> URL: https://issues.apache.org/jira/browse/YARN-10585
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Gergely Pollak
>Assignee: Gergely Pollak
>Priority: Major
> Attachments: YARN-10585.001.patch, YARN-10585.002.patch
>
>
> To make transition easier we need to create tooling to support the migration
> effort. The first step is to create a class which can migrate from legacy to
> the new JSON format.
--
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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17269431#comment-17269431
]
Hadoop QA commented on YARN-10585:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 1m
24s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} || ||
| {color:green}+1{color} | {color:green} dupname {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} No case conflicting files
found. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch does not contain any
@author tags. {color} |
| {color:green}+1{color} | {color:green} {color} | {color:green} 0m 0s{color}
| {color:green}test4tests{color} | {color:green} The patch appears to include 3
new or modified test files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 23m
49s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
6s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
59s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
40s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
58s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
18m 30s{color} | {color:green}{color} | {color:green} branch has no errors when
building and testing our client artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
39s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
34s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:blue}0{color} | {color:blue} spotbugs {color} | {color:blue} 1m
52s{color} | {color:blue}{color} | {color:blue} Used deprecated FindBugs
config; considering switching to SpotBugs. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m
50s{color} | {color:green}{color} | {color:green} trunk passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
55s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
1s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m
1s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
53s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
53s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
0m 35s{color} |
{color:orange}https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/532/artifact/out/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt{color}
| {color:orange}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:
The patch generated 1 new + 5 unchanged - 0 fixed = 6 total (was 5) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
58s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch has no whitespace
issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
16m 36s{color} | {color:green}{color} | {color:green} patch has no errors when
building and testing our client artifacts. {color} |
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[ https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17269316#comment-17269316 ] Gergely Pollak commented on YARN-10585: --- Fixed the unit test failures, improved the conversion of rules like root.deep.%primary_group.%secondary_group, which was not supported by the legacy mapping rule engine, but the new mapping engine supports it even with the legacy format, so the converter should too. > Create a class which can convert from legacy mapping rule format to the new > JSON format > --- > > Key: YARN-10585 > URL: https://issues.apache.org/jira/browse/YARN-10585 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Attachments: YARN-10585.001.patch, YARN-10585.002.patch > > > To make transition easier we need to create tooling to support the migration > effort. The first step is to create a class which can migrate from legacy to > the new JSON format. -- 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
[jira] [Commented] (YARN-10585) Create a class which can convert from legacy mapping rule format to the new JSON format
[
https://issues.apache.org/jira/browse/YARN-10585?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17269089#comment-17269089
]
Hadoop QA commented on YARN-10585:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 26m
7s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} || ||
| {color:green}+1{color} | {color:green} dupname {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} No case conflicting files
found. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch does not contain any
@author tags. {color} |
| {color:green}+1{color} | {color:green} {color} | {color:green} 0m 0s{color}
| {color:green}test4tests{color} | {color:green} The patch appears to include 1
new or modified test files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 21m
22s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
2s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
52s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
38s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
59s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
16m 32s{color} | {color:green}{color} | {color:green} branch has no errors when
building and testing our client artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
42s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
40s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:blue}0{color} | {color:blue} spotbugs {color} | {color:blue} 1m
49s{color} | {color:blue}{color} | {color:blue} Used deprecated FindBugs
config; considering switching to SpotBugs. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m
46s{color} | {color:green}{color} | {color:green} trunk passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
55s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
56s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
56s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
47s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
47s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
0m 30s{color} |
{color:orange}https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/528/artifact/out/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt{color}
| {color:orange}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:
The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
49s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch has no whitespace
issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
15m 0s{color} | {color:green}{color} | {color:green} patch has no errors when
building and testing our client artifacts. {color} |
