You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cnauroth (via GitHub)" <gi...@apache.org> on 2025/03/05 22:42:40 UTC
[PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
cnauroth opened a new pull request, #50173:
URL: https://github.com/apache/spark/pull/50173
### What changes were proposed in this pull request?
While verifying Spark 4.0.0 RC2, I consistently saw YARN test `AmIpFilterSuite#testProxyUpdate` failing in my environment. The test is written to eventually expect a `ServletException` from `getProxyAddresses` after 5 seconds of retries, but I never received this exception.
This test and the corresponding `AmIpFilter` were introduced in [SPARK-48238](https://issues.apache.org/jira/browse/SPARK-48238) as a fork of the Hadoop implementation to resolve a dependency conflict. However, it seems this test had a small bug in the way it was adapted into the Spark codebase. The `AmIpFilter#getProxyAddresses()` logic may either return an empty set or throw a `ServletException` if it can't find any valid configured proxies. The Hadoop test's assertion allows for either of these conditions:
https://github.com/apache/hadoop/blob/rel/release-3.4.0/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/amfilter/TestAmFilter.java#L212-L222
```
// waiting for configuration update
GenericTestUtils.waitFor(new Supplier<Boolean>() {
@Override
public Boolean get() {
try {
return filter.getProxyAddresses().isEmpty();
} catch (ServletException e) {
return true;
}
}
}, 500, updateInterval);
```
The version in Spark strictly requires an exception to be thrown:
https://github.com/apache/spark/blob/v4.0.0-rc2/resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala#L163-L168
```
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
assertThrows[ServletException] {
filter.getProxyAddresses.isEmpty
}
}
```
The test involves updating the proxy configuration to use "unknownhost" as an invalid proxy. In my network, there is actually a host named "unknownhost", but it only has an IPv6 address, and I only have an IPv4 address. This causes a "network unreachable" error instead of "unknown host", resulting in an empty set instead of an exception.
This pull request changes the Spark test to be consistent with the Hadoop test, allowing either condition to succeed.
### Why are the changes needed?
Maintain consistency with the intent of the original Hadoop test and ensure it can pass in any network setup.
### Does this PR introduce _any_ user-facing change?
No. The changes are in tests only.
### How was this patch tested?
Existing tests pass in my environment after this change.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982630520
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
It sounds like the intent of this proposed change is to succeed specifically if there is a `ServletException`, but fail for other exception types. I don't think we need code changes to achieve this though. Exceptions different from `ServletException` propagate from the test and cause a test failure.
To confirm this, I just tried introducing a `throw new RuntimeException()` in my local copy AmIpFilter.java. As I expected, the test failed.
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
It sounds like the intent of this proposed change is to succeed specifically if there is a `ServletException`, but fail for other exception types. I don't think we need code changes to achieve this though. Exceptions different from `ServletException` propagate from the test and cause a test failure.
To confirm this, I just tried introducing a `throw new RuntimeException()` in my local copy of AmIpFilter.java. As I expected, the test failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982607868
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
maybe
```suggestion
case e: ServletException => // do nothing
case t => fail()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982653029
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
Review Comment:
Thank you, I understand the feedback now. As it turns out, the result is a non-empty set of addresses, containing the IPv6 address. `eventually` is allowing this to pass, because any return value is considered successful.
I'll investigate this more and update the patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982635542
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
Review Comment:
I understand that, my point is that unlike `GenericTestUtils.waitFor` tests the body returning `true` or `false`, `eventually` tests the block whether to throw an exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982638623
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
yes, correct, but `true` is unnecessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982630520
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
It sounds like the intent of this proposed change is to succeed specifically if there is a `ServletException`, but fail for other exception types. I don't think we need code changes to achieve this though. Exceptions different from `ServletException` propagate from the test and cause a test failure.
To confirm this, I just tried introducing a `throw RuntimeException()` in my local copy AmIpFilter.java. As I expected, the test failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on PR #50173:
URL: https://github.com/apache/spark/pull/50173#issuecomment-2702251438
@pan3793 and @LuciferYang , this relates to code you added/reviewed in #46611 . Would you please review this test fix? Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982607868
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
```suggestion
case e: ServletException => // do nothing
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #50173: [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks
URL: https://github.com/apache/spark/pull/50173
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on PR #50173:
URL: https://github.com/apache/spark/pull/50173#issuecomment-2705058426
FYI, apache/hadoop#7478 has a follow-up to keep in sync with the changes here to randomize the fake host name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982737452
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
Review Comment:
After another look, this definitely should be wrapped in an assert. I have pushed up the change.
I also changed the invalid "unknownhost" with some additional randomization to increase the likelihood that we are referring to a host that does not exist. For this part of the patch, I am also planning to commit it back to the original Hadoop test code that Spark forked.
Thanks for the great code review, @pan3793 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982628947
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
Review Comment:
No, there are two possible outcomes, and both must be considered valid for the assertion.
1) "unknownhost" does not exist on the network. The logic of `AmIpFilter` detects this as an `UnknownHostException`, and the end result is an empty set of viable proxy addresses. `AmIpFilter` then throws `ServletException`.
2) "unknownhost" does exist on the network. It does resolve through DNS. However, there is no routable path to the host. In my case, as mentioned in the original description, that's because I only have an IPv4 address and the "unknownhost" on my network only has an IPv6 address. In this case, no exception is thrown, and the expectation is for `getProxyAddresses()` to return an empty set.
This logic is perhaps a little unexpected, but I believe the intent of [SPARK-48238](https://issues.apache.org/jira/browse/SPARK-48238) was to match the pre-defined behavior of Hadoop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #50173:
URL: https://github.com/apache/spark/pull/50173#issuecomment-2703134279
Merged into master and branch-4.0. Thanks @cnauroth @pan3793 and @dongjoon-hyun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982606487
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
Review Comment:
should it be `assert(filter.getProxyAddresses.isEmpty)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on code in PR #50173:
URL: https://github.com/apache/spark/pull/50173#discussion_r1982738051
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/AmIpFilterSuite.scala:
##########
@@ -162,8 +162,10 @@ class AmIpFilterSuite extends SparkFunSuite {
assert(!filter.getProxyAddresses.isEmpty)
// waiting for configuration update
eventually(timeout(5.seconds), interval(500.millis)) {
- assertThrows[ServletException] {
+ try {
filter.getProxyAddresses.isEmpty
+ } catch {
+ case e: ServletException => true
Review Comment:
I have pushed up changes to incorporate this suggestion. Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51408][YARN][TESTS] AmIpFilterSuite#testProxyUpdate fails in some networks [spark]
Posted by "cnauroth (via GitHub)" <gi...@apache.org>.
cnauroth commented on PR #50173:
URL: https://github.com/apache/spark/pull/50173#issuecomment-2704380243
Thank you, @LuciferYang !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org