[jira] [Comment Edited] (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=17278337#comment-17278337 ] Szilard Nemeth edited comment on YARN-10585 at 2/3/21, 8:10 PM: 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 modifies git's commit 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 exceptional situation. I have 200+ added 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 [~shuzirra] introduced and the coverage was more than enough. Such an NPE would have surfaced during the UT execution as well. [~sunil.gov...@gmail.com] Please chime in for the topic of how to fix this: in a follow-up or reopening this one, please share your thoughts about pros/cons. Thanks was (Author: snemeth): 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 modifies git's commit 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 exceptional situation. I have 200+ added 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 [~shuzirra] 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)
[jira] [Comment Edited] (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=17278337#comment-17278337 ] Szilard Nemeth edited comment on YARN-10585 at 2/3/21, 8:10 PM: 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 modifies git's commit 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 exceptional situation. I have 200+ added 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 [~shuzirra] introduced and the coverage was more than enough. Such an NPE would have surfaced during the UT execution as well. [~sunilg] Please chime in for the topic of how to fix this: in a follow-up or reopening this one, please share your thoughts about pros/cons. Thanks was (Author: snemeth): 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 modifies git's commit 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 exceptional situation. I have 200+ added 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 [~shuzirra] introduced and the coverage was more than enough. Such an NPE would have surfaced during the UT execution as well. [~sunil.gov...@gmail.com] Please chime in for the topic of how to fix this: in a follow-up or reopening this one, please share your thoughts about pros/cons. Thanks > 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
[jira] [Comment Edited] (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=17278337#comment-17278337 ] Szilard Nemeth edited comment on YARN-10585 at 2/3/21, 8:09 PM: 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 modifies git's commit 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 exceptional situation. I have 200+ added 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 [~shuzirra] introduced and the coverage was more than enough. Such an NPE would have surfaced during the UT execution as well. was (Author: snemeth): 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 modifies git's commit 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 exceptional situation. I have 200+ added 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:
[jira] [Comment Edited] (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=17278337#comment-17278337 ] Szilard Nemeth edited comment on YARN-10585 at 2/3/21, 8:08 PM: 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 modifies git's commit 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 exceptional situation. I have 200+ added 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. was (Author: snemeth): 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 modifies git's commit 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:
[jira] [Comment Edited] (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=17278337#comment-17278337 ] Szilard Nemeth edited comment on YARN-10585 at 2/3/21, 8:08 PM: 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 modifies git's commit 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. was (Author: snemeth): 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:
[jira] [Comment Edited] (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=17271371#comment-17271371 ] Benjamin Teke edited comment on YARN-10585 at 1/25/21, 3:34 PM: Thank [~shuzirra] for working on this. Based on your answers to [~gandras]'s comments the patch looks good to me, so generally +1 (non-binding) from my side. Another patch might be needed because the 002 generates license warnings and has a checkstyle issue. was (Author: bteke): 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] [Comment Edited] (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=17271297#comment-17271297
]
Gergely Pollak edited comment on YARN-10585 at 1/25/21, 1:01 PM:
-
[~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.
was (Author: shuzirra):
[~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
