-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MAPREDUCE-7434. Fix ShuffleHandler tests. #5426
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
assertTrue(String.format("Received string '%s' should contain " + | ||
"message '%s'", receivedString, message), | ||
receivedString.contains(message)); | ||
assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a valid change ?. Couldn't verify it locally( as it doesn't have NativeIO either).
Except this evrything LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should return with 500 Internal Server Error. (HADOOP-15327 introduced the HTTP_OK response checking but that was a wrong behaviour.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests seems to be passing now, do you folks intend to catch the committers involved in the original ticket for the review? if they aren't available, then you need to give me details around the fix. Like this is the issue-> this is why this change is required stuff, for me to commit this, if those folks aren't available
Thanks @tomicooler for working on this! |
Description of PR
Fix the user for the ShuffleHandler tests.
How was this patch tested?
I tested manually with the docker environment:
./start-build-env.sh
The 3 failures could be reproduced.
The tests are green with this change.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?