You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "yl09099 (via GitHub)" <gi...@apache.org> on 2024/07/26 05:27:21 UTC
[PR] [#1927] The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
yl09099 opened a new pull request, #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965
### What changes were proposed in this pull request?
1. Modify NPE bugs;
2. Modify the Vue3's CompositionAPI.
3. Handling the request failed.
4. Optimize page display.
### Why are the changes needed?
Fix: #1954
### Does this PR introduce _any_ user-facing change?
Yes.
### How was this patch tested?
UT.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): Fix NPE issues in dashboard [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma merged PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#discussion_r1692609115
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -46,47 +52,56 @@ protected String rewriteTarget(HttpServletRequest clientRequest) {
return null;
}
String targetAddress;
- if (clientRequest.getHeader("serverType").equals("coordinator")) {
- targetAddress = coordinatorServerAddressesMap.get(clientRequest.getHeader("targetAddress"));
- if (targetAddress == null) {
- // Get random one from coordinatorServerAddressesMap
- targetAddress = coordinatorServerAddressesMap.values().iterator().next();
- }
+ String requestServerType =
+ clientRequest.getHeader(HEADER_REQUEST_SERVER_TYPE) != null
+ && REQUEST_SERVER_TYPE_COORDINATOR.equalsIgnoreCase(
+ clientRequest.getHeader(HEADER_REQUEST_SERVER_TYPE))
+ ? REQUEST_SERVER_TYPE_COORDINATOR
+ : REQUEST_SERVER_TYPE_SERVER;
+ if (requestServerType.equalsIgnoreCase(REQUEST_SERVER_TYPE_COORDINATOR)) {
+ targetAddress =
+ coordinatorServerAddressesMap.get(clientRequest.getHeader(HEADER_TARGET_ADDRESS));
} else {
- targetAddress = clientRequest.getHeader("targetAddress");
+ targetAddress = clientRequest.getHeader(HEADER_TARGET_ADDRESS);
}
StringBuilder target = new StringBuilder();
- if (targetAddress.endsWith("/")) {
- targetAddress = targetAddress.substring(0, targetAddress.length() - 1);
- }
target.append(targetAddress).append("/api").append(clientRequest.getPathInfo());
String query = clientRequest.getQueryString();
if (query != null) {
target.append("?").append(query);
}
+ LOG.info(target.toString());
return target.toString();
}
- @Override
- protected void onProxyRewriteFailed(
- HttpServletRequest clientRequest, HttpServletResponse clientResponse) {}
-
+ /**
+ * If the proxy address fails to be requested, 403 is returned and the front-end handles the
+ * exception.
+ *
+ * @param clientRequest
Review Comment:
Its ok.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): Fix NPE issues in dashboard [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#issuecomment-2260044505
Merged to master. Thanks for your contribution. @yl09099 Thanks for your reviews. @maobaolong @jerqi
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#issuecomment-2252040989
## Test Results
2 732 files ±0 2 732 suites ±0 9h 47m 3s :stopwatch: +8s
970 tests ±0 969 :white_check_mark: ±0 1 :zzz: ±0 0 :x: ±0
12 159 runs ±0 12 144 :white_check_mark: ±0 15 :zzz: ±0 0 :x: ±0
Results for commit e0ab8bae. ± Comparison against base commit 69522109.
[test-results]:data:application/gzip;base64,H4sIAEw9o2YC/03MSQ7DIAyF4atErLsIZu5lKkOIhJqhIrCKevc6Vabl+zD/yvo0xIU9GzACHg1bair33dWMJc0TiVAAgojey3bhTHus11JD2Ei7i97pQ8RP6DENBNenmPOcd8l12pocuHL7PKJkUp62V9UBt+p/36NhHsdUaLDYorceIwaMYAEldN4qH6V0XnPTBqO1cL1l3x86/9QUEgEAAA==
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#issuecomment-2252007306
@jerqi @xianjingfeng @rickyma @kuszz @maobaolong PATL
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#discussion_r1692572673
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -46,47 +52,56 @@ protected String rewriteTarget(HttpServletRequest clientRequest) {
return null;
}
String targetAddress;
- if (clientRequest.getHeader("serverType").equals("coordinator")) {
- targetAddress = coordinatorServerAddressesMap.get(clientRequest.getHeader("targetAddress"));
- if (targetAddress == null) {
- // Get random one from coordinatorServerAddressesMap
- targetAddress = coordinatorServerAddressesMap.values().iterator().next();
- }
+ String requestServerType =
+ clientRequest.getHeader(HEADER_REQUEST_SERVER_TYPE) != null
+ && REQUEST_SERVER_TYPE_COORDINATOR.equalsIgnoreCase(
+ clientRequest.getHeader(HEADER_REQUEST_SERVER_TYPE))
+ ? REQUEST_SERVER_TYPE_COORDINATOR
+ : REQUEST_SERVER_TYPE_SERVER;
+ if (requestServerType.equalsIgnoreCase(REQUEST_SERVER_TYPE_COORDINATOR)) {
+ targetAddress =
+ coordinatorServerAddressesMap.get(clientRequest.getHeader(HEADER_TARGET_ADDRESS));
} else {
- targetAddress = clientRequest.getHeader("targetAddress");
+ targetAddress = clientRequest.getHeader(HEADER_TARGET_ADDRESS);
}
StringBuilder target = new StringBuilder();
- if (targetAddress.endsWith("/")) {
- targetAddress = targetAddress.substring(0, targetAddress.length() - 1);
- }
target.append(targetAddress).append("/api").append(clientRequest.getPathInfo());
String query = clientRequest.getQueryString();
if (query != null) {
target.append("?").append(query);
}
+ LOG.info(target.toString());
return target.toString();
}
- @Override
- protected void onProxyRewriteFailed(
- HttpServletRequest clientRequest, HttpServletResponse clientResponse) {}
-
+ /**
+ * If the proxy address fails to be requested, 403 is returned and the front-end handles the
+ * exception.
+ *
+ * @param clientRequest
Review Comment:
These java docs seems generated by development tools, but uncompleted?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#issuecomment-2252146836
> @yl09099 Why the color of each button are different between coordinator page and server node page
OK,Let me change the color.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#issuecomment-2252152170
> @yl09099 Why the color of each button are different between coordinator page and server node page
Modified, color consistent.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on code in PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#discussion_r1692569804
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -22,16 +22,22 @@
import javax.servlet.http.HttpServletResponse;
import com.google.common.base.Preconditions;
-import org.eclipse.jetty.client.api.Request;
-import org.eclipse.jetty.client.api.Response;
+import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.proxy.ProxyServlet;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class WebProxyServlet extends ProxyServlet {
private static final Logger LOG = LoggerFactory.getLogger(WebProxyServlet.class);
+ /** The key of the request header. */
+ private static final String TARGETADDRESS = "targetAddress";
+ private static final String REQUESTSERVERTYPE = "requestServerType";
+ /** The value of the request header. */
+ private static final String COORDINATOR = "coordinator";
+
+ private static final String SERVER = "server";
Review Comment:
> REQUEST_SERVER_TYPE_SERVER
done.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#issuecomment-2252116976
@yl09099 Why the color of each button are different between coordinator page and server node page
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#discussion_r1692561026
##########
conf/local_dev/dashboard.conf:
##########
@@ -17,4 +17,4 @@
rss.dashboard.http.port 19997
# change to the actual coordinator web addresses
-coordinator.web.address http://localhost:19998,http://localhost:19998
+coordinator.web.address http://127.0.0.1:19998,http://localhost:19999
Review Comment:
You can move this conf into a place out of git version control?
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -22,16 +22,22 @@
import javax.servlet.http.HttpServletResponse;
import com.google.common.base.Preconditions;
-import org.eclipse.jetty.client.api.Request;
-import org.eclipse.jetty.client.api.Response;
+import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.proxy.ProxyServlet;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class WebProxyServlet extends ProxyServlet {
private static final Logger LOG = LoggerFactory.getLogger(WebProxyServlet.class);
+ /** The key of the request header. */
+ private static final String TARGETADDRESS = "targetAddress";
Review Comment:
TARGETADDRESS -> `HEADER_TARGET_ADDRESS ` would be better
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -22,16 +22,22 @@
import javax.servlet.http.HttpServletResponse;
import com.google.common.base.Preconditions;
-import org.eclipse.jetty.client.api.Request;
-import org.eclipse.jetty.client.api.Response;
+import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.proxy.ProxyServlet;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class WebProxyServlet extends ProxyServlet {
private static final Logger LOG = LoggerFactory.getLogger(WebProxyServlet.class);
+ /** The key of the request header. */
+ private static final String TARGETADDRESS = "targetAddress";
+ private static final String REQUESTSERVERTYPE = "requestServerType";
Review Comment:
The same `REQUESTSERVERTYPE ` -> `HEADER_REQUEST_SERVER_TYPE`
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -22,16 +22,22 @@
import javax.servlet.http.HttpServletResponse;
import com.google.common.base.Preconditions;
-import org.eclipse.jetty.client.api.Request;
-import org.eclipse.jetty.client.api.Response;
+import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.proxy.ProxyServlet;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class WebProxyServlet extends ProxyServlet {
private static final Logger LOG = LoggerFactory.getLogger(WebProxyServlet.class);
+ /** The key of the request header. */
+ private static final String TARGETADDRESS = "targetAddress";
+ private static final String REQUESTSERVERTYPE = "requestServerType";
+ /** The value of the request header. */
+ private static final String COORDINATOR = "coordinator";
+
+ private static final String SERVER = "server";
Review Comment:
REQUEST_SERVER_TYPE_SERVER
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -22,16 +22,22 @@
import javax.servlet.http.HttpServletResponse;
import com.google.common.base.Preconditions;
-import org.eclipse.jetty.client.api.Request;
-import org.eclipse.jetty.client.api.Response;
+import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.proxy.ProxyServlet;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class WebProxyServlet extends ProxyServlet {
private static final Logger LOG = LoggerFactory.getLogger(WebProxyServlet.class);
+ /** The key of the request header. */
+ private static final String TARGETADDRESS = "targetAddress";
+ private static final String REQUESTSERVERTYPE = "requestServerType";
+ /** The value of the request header. */
+ private static final String COORDINATOR = "coordinator";
Review Comment:
REQUEST_SERVER_TYPE_COORDINATOR
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1927] fix(dashboard): The introduction of serverType causes all NPE except GET requests. [incubator-uniffle]
Posted by "yl09099 (via GitHub)" <gi...@apache.org>.
yl09099 commented on code in PR #1965:
URL: https://github.com/apache/incubator-uniffle/pull/1965#discussion_r1692592434
##########
dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java:
##########
@@ -46,47 +52,56 @@ protected String rewriteTarget(HttpServletRequest clientRequest) {
return null;
}
String targetAddress;
- if (clientRequest.getHeader("serverType").equals("coordinator")) {
- targetAddress = coordinatorServerAddressesMap.get(clientRequest.getHeader("targetAddress"));
- if (targetAddress == null) {
- // Get random one from coordinatorServerAddressesMap
- targetAddress = coordinatorServerAddressesMap.values().iterator().next();
- }
+ String requestServerType =
+ clientRequest.getHeader(HEADER_REQUEST_SERVER_TYPE) != null
+ && REQUEST_SERVER_TYPE_COORDINATOR.equalsIgnoreCase(
+ clientRequest.getHeader(HEADER_REQUEST_SERVER_TYPE))
+ ? REQUEST_SERVER_TYPE_COORDINATOR
+ : REQUEST_SERVER_TYPE_SERVER;
+ if (requestServerType.equalsIgnoreCase(REQUEST_SERVER_TYPE_COORDINATOR)) {
+ targetAddress =
+ coordinatorServerAddressesMap.get(clientRequest.getHeader(HEADER_TARGET_ADDRESS));
} else {
- targetAddress = clientRequest.getHeader("targetAddress");
+ targetAddress = clientRequest.getHeader(HEADER_TARGET_ADDRESS);
}
StringBuilder target = new StringBuilder();
- if (targetAddress.endsWith("/")) {
- targetAddress = targetAddress.substring(0, targetAddress.length() - 1);
- }
target.append(targetAddress).append("/api").append(clientRequest.getPathInfo());
String query = clientRequest.getQueryString();
if (query != null) {
target.append("?").append(query);
}
+ LOG.info(target.toString());
return target.toString();
}
- @Override
- protected void onProxyRewriteFailed(
- HttpServletRequest clientRequest, HttpServletResponse clientResponse) {}
-
+ /**
+ * If the proxy address fails to be requested, 403 is returned and the front-end handles the
+ * exception.
+ *
+ * @param clientRequest
Review Comment:
> These java docs seems generated by development tools, but uncompleted?
I made it easy, but it was done.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org