-
Notifications
You must be signed in to change notification settings - Fork 9.1k
YARN-11153. Make proxy server support YARN federation. #4314
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
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help me review this PR? |
@Override | ||
public void setUp() throws Exception { | ||
super.setUp(); | ||
conf.setBoolean(YarnConfiguration.FEDERATION_ENABLED, true); |
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.
Do we want to extend some tests just for federation or TestWebAppProxyServer is now generic enough?
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.
It's only for test the basic function (For example: start or stop and so on) of WebAppProxyServer in federation mode.
For me, I think TestWebAppProxyServer is not generic enough, because that UT only test start, stop and bindaddress. I don't know whether is necessary to add some integration testing so that we can test proxy app web link?
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.
@goiri I remove TestWebProxyServerFed. It is unnecessary for us to test startup. I add new UT TestWebAppProxyServletFed, will test proxy link in federation mode.
...-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestFedAppReportFetcher.java
Outdated
Show resolved
Hide resolved
...-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestFedAppReportFetcher.java
Outdated
Show resolved
Hide resolved
💔 -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. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri I reconstruct my UT, add new test class TestWebAppProxyServletFed. It will simulate the proxy link in federation mode. Can you please review it again? |
@goiri Is this proposal OK? YARN-11154 is blocked by this. Can you please review this again? |
...-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestFedAppReportFetcher.java
Outdated
Show resolved
Hide resolved
...-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestFedAppReportFetcher.java
Outdated
Show resolved
Hide resolved
...-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestFedAppReportFetcher.java
Outdated
Show resolved
Hide resolved
...-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestFedAppReportFetcher.java
Outdated
Show resolved
Hide resolved
...-server-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/AppReportFetcher.java
Outdated
Show resolved
Hide resolved
...r-web-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServlet.java
Outdated
Show resolved
Hide resolved
...eb-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServletFed.java
Outdated
Show resolved
Hide resolved
...eb-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServletFed.java
Outdated
Show resolved
Hide resolved
...eb-proxy/src/test/java/org/apache/hadoop/yarn/server/webproxy/TestWebAppProxyServletFed.java
Outdated
Show resolved
Hide resolved
proxyServer = new HttpServer2.Builder() | ||
.setName("proxy") | ||
.addEndpoint( | ||
URI.create(WebAppUtils.getHttpSchemePrefix(conf) + bindAddress |
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.
Isn't there an API with the subparts?
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.
Isn't there an API with the subparts?
Yes, there are no api like URI(String scheme, String host, int port).
But there is an API URI(String scheme, String userInfo, String host, int port,String path, String query, String fragment). In this construct method, firstly construct a string, then parse. I think it is not better than URL(String spec)
🎊 +1 overall
This message was automatically generated. |
...erver-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/WebAppProxyServlet.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationStateStoreFacade.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
...rver-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/FedAppReportFetcher.java
Show resolved
Hide resolved
Is there any problem about this PR? How about merge trunk? Then I will continue YARN-11153. @goiri @slfan1989 |
Thank you very much for your contribution, overall it looks fine, but some code is better to extract. |
...erver-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/WebAppProxyServlet.java
Outdated
Show resolved
Hide resolved
...erver-web-proxy/src/main/java/org/apache/hadoop/yarn/server/webproxy/WebAppProxyServlet.java
Outdated
Show resolved
Hide resolved
🎊 +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. |
@zhengchenyu Thanks for the contribution! LGTM. |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you double check this pr again? Thank you very much! |
@goiri Thank you very much for your help in reviewing the code! @zhengchenyu Thank you for your contribution! |
Description of PR
Can't get running appmaster's web app in federation mode. (Detail message seen: https://issues.apache.org/jira/browse/YARN-10775)
Two step to solve this problem :
(a) make proxy server support federation.
(b) make router support proxy server.
It is the first step(a), detail message see: https://issues.apache.org/jira/browse/YARN-11153
How was this patch tested?
manually test in our cluster and unit test.
For code changes:
make proxyserver support federation