HADOOP-16649. hadoop_add_param function : change regexp test by iterative equality test #2385
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tive equality test on each value.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Just to save everyone a lot of time and suffering: This approach will break a lot of things in very unexpected ways (doing a search on everywhere hadoop_add_params is called should make this clear). hadoop_add_param was specifically built for partial matches because the HADOOP_OPTS command line can't really do exact matches and this was a quick way to prevent duplicate options. The unit test failure in hadoop_finalize_hadoop_heap was intended to provide a hint that "yar they be dragons here." I should have written better tests, but given it took like 2 years just to get most of this code in over the total @#$@#$ that was in hadoop 2.x ... When I wrote the code originally, we didn't have a need for exact matches anywhere (HADOOP_OPTIONAL_TOOLS wasn't written yet). It was written and committed to 3.x. Then the HADOOP_OPTIONAL_TOOLS code was written but that would be the only place where an exact match would be useful and we didn't have any sooo... I just re-used hadoop_add_param with the (clearly faulty) assumption that people would test their code on Hadoop 3.x. But since the azure team didn't bother to test with hadoop 3.x until it was too late... At this point, I was getting tired of the Hadoop politics and bailed, leaving this furball hanging around. Anyway, the real fix for this is to convert HADOOP_OPTIONAL_TOOLS to an array and then do an exact match, looping over the array. I think there is code to do that now. Might need some new helper code to do comma-delimited -> array but that shouldn't be hard. |
|
Hi The new code keeps the privious pattern matching test (sorry for the misleading title) : it will accept a new param only if it is not found using the previous pattern testing and not found using an equality testing (to address the hadoop-azure hadoop-azure-datalake) I also added a specific test for the pattern matching : @test "hadoop_add_param (HADOOP-16649 c )" { |
|
Sad to see no evolution of this PR since 1 month. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The bug HADOOP-16649 is due to a regexp test : hadoop-azure is detected as already present when hadoop-azure-datalake is present.
This PR contains an update for hadoop_add_param function to address the problem, and the corresponding bats test.