Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


pan3793 commented on code in PR #46611:
URL: https://github.com/apache/spark/pull/46611#discussion_r1605495540


##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   I was consider to port the code as-is(also keeping package and class name) 
as we did in the `thriftserver` module for `hive-service`, while it would cause 
class conflicts because `hadoop-client-minicluster`(present in test scope) 
ships those classes too. Meanwhile, I found an existing 
`YarnProxyRedirectFilter` which is much simpler and is not coupled with Hadoop 
classes. So I rename the class to `YarnAMIpFilter`, rewrite it in Scala, and 
put it alongside `YarnProxyRedirectFilter`. UT `YarnAMIpFilterSuite` is ported 
too to ensure the correctness of rewriting.
   
   @cloud-fan if we pursue sync with Hadoop upstream, I can convert it back to 
Java classes, and reuse the Hadoop code as much as possible. For example, use 
`org.apache.hadoop.yarn.webapp.hamlet2.Hamlet` to construct the HTML page in 
`sendRedirect` instead of writing by hand as we did in 
`YarnProxyRedirectFilter`.



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


pan3793 commented on code in PR #46611:
URL: https://github.com/apache/spark/pull/46611#discussion_r1605495540


##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   I was consider to port the code as-is(also keeping package and class name) 
as we did in the `thriftserver` module for `hive-service`, while it would cause 
class conflicts because `hadoop-client-minicluster`(present in test scope) 
ships those classes too. Meanwhile, I found an existing 
`YarnProxyRedirectFilter` which is much simpler and is not coupled with Hadoop 
classes. So I rename the class to `YarnAMIpFilter`, rewrote it in Scala, and 
put it alongside `YarnProxyRedirectFilter`. A UT `YarnAMIpFilterSuite` is 
ported too to ensure the correctness of rewriting.
   
   @cloud-fan if we pursue sync with Hadoop upstream, I can convert it back to 
Java classes, and reuse the Hadoop code as much as possible. For example, use 
`org.apache.hadoop.yarn.webapp.hamlet2.Hamlet` to construct the HTML page in 
`sendRedirect` instead of writing by hand as we did in 
`YarnProxyRedirectFilter`.



##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   I was consider to port the code as-is(also keeping package and class name) 
as we did in the `thriftserver` module for `hive-service`, while it would cause 
class conflicts because `hadoop-client-minicluster`(present in test scope) 
ships those classes too. Meanwhile, I found an existing 
`YarnProxyRedirectFilter` which is much simpler and is not coupled with Hadoop 
classes. So I rename the class to `YarnAMIpFilter`, rewrite it in Scala, and 
put it alongside `YarnProxyRedirectFilter`. A UT `YarnAMIpFilterSuite` is 
ported too to ensure the correctness of rewriting.
   
   @cloud-fan if we pursue sync with Hadoop upstream, I can convert it back to 
Java 

Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


pan3793 commented on code in PR #46611:
URL: https://github.com/apache/spark/pull/46611#discussion_r1605495540


##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   I was consider to port the code as-is(also keeping package and class name) 
as we did in the `thriftserver` module for `hive-service`, while it would cause 
class conflicts because `hadoop-client-minicluster`(present in test scope) 
ships those classes too. Meanwhile, I found an existing 
`YarnProxyRedirectFilter` which is much simpler and is not coupled with Hadoop 
classes. So I rename the class to `YarnAMIpFilter`, and put it alongside 
`YarnProxyRedirectFilter`. A UT `YarnAMIpFilterSuite` is ported too to ensure 
the correctness of rewriting.
   
   @cloud-fan if we pursue sync with Hadoop upstream, I can convert it back to 
Java classes, and reuse the Hadoop code as much as possible. For example, use 
`org.apache.hadoop.yarn.webapp.hamlet2.Hamlet` to construct the HTML page in 
`sendRedirect` instead of writing by hand as we did in 
`YarnProxyRedirectFilter`.



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


pan3793 commented on code in PR #46611:
URL: https://github.com/apache/spark/pull/46611#discussion_r1605458503


##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   This is a functional equivalent rewrite. Simplify the code by converting to 
Scala and merging code from several Hadoop Java classes.
   



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


cloud-fan commented on code in PR #46611:
URL: https://github.com/apache/spark/pull/46611#discussion_r1605076750


##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   If the original code is Java, I'd prefer we also add a Java file in Spark, 
so that it's easier to keep the code in sync.



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


cloud-fan commented on PR #46611:
URL: https://github.com/apache/spark/pull/46611#issuecomment-2117700736

   This looks like a good idea to get rid of the conflicting dependency. We 
should clarify 
https://github.com/apache/spark/pull/46611#discussion_r1605001322 though.


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


tgravescs commented on code in PR #46611:
URL: https://github.com/apache/spark/pull/46611#discussion_r1605001322


##
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAMIpFilter.scala:
##
@@ -0,0 +1,256 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.deploy.yarn
+
+import java.io.IOException
+import java.net.{HttpURLConnection, InetAddress, MalformedURLException, 
UnknownHostException, URL}
+import java.security.Principal
+import java.util.concurrent.TimeUnit
+
+import jakarta.servlet.{Filter, FilterChain, FilterConfig, ServletException, 
ServletRequest, ServletResponse}
+import jakarta.servlet.http.{HttpServletRequest, HttpServletRequestWrapper, 
HttpServletResponse}
+import org.apache.commons.lang3.StringUtils
+import org.apache.hadoop.security.UserGroupInformation
+
+import org.apache.spark.internal.Logging
+
+// This class is inspired by 
org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter

Review Comment:
   can you clarify this, is it just  copy converted to scala or did you 
add/change functionality?



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-17 Thread via GitHub


pan3793 commented on PR #46611:
URL: https://github.com/apache/spark/pull/46611#issuecomment-2117258617

   cc @cloud-fan @dongjoon-hyun @LuciferYang @srowen 


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-16 Thread via GitHub


pan3793 commented on PR #46611:
URL: https://github.com/apache/spark/pull/46611#issuecomment-2116553658

   > ... supposedly the workaround will be removed when the Yarn side upgrades 
their J2EE?
   
   I suppose not. According to the discussion in 
https://github.com/apache/spark/pull/31642 
`org.apache.hadoop.yarn.server.webproxy.amfilter.AmIpFilter` should be part of 
`hadoop-client-api` but actually not, and
   
   > Agree that in the long term we should either: 1) consider to re-implement 
the logic in Spark which allows us to get away from server-side dependency in 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-16 Thread via GitHub


HiuKwok commented on PR #46611:
URL: https://github.com/apache/spark/pull/46611#issuecomment-2116550519

   The patch makes sense to me, and supposedly the workaround will be removed 
when the Yarn side upgrades their J2EE?


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



Re: [PR] [SPARK-48238][BUILD][YARN] Replace AmIpFilter with re-implemented YarnAMIpFilter [spark]

2024-05-16 Thread via GitHub


mridulm commented on PR #46611:
URL: https://github.com/apache/spark/pull/46611#issuecomment-2116089785

   +CC @tgravescs 


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org