You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "zhaohehuhu (via GitHub)" <gi...@apache.org> on 2023/06/20 08:03:20 UTC
[GitHub] [kyuubi] zhaohehuhu opened a new pull request, #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
zhaohehuhu opened a new pull request, #4983:
URL: https://github.com/apache/kyuubi/pull/4983
close #4982
The targetURL already got the query string and kyuubi server can forward request to right page, eg: /engine-ui/spark-43efae88d766226a-driver-svc.kyuubi-dlssjc-canned.svc:4045/jobs/job/?id=4
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zwangsheng commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1234906620
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -30,14 +30,15 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
+ val queryString = getQueryString(request)
var targetURL = "/no-ui-error"
extractTargetAddress(requestURI).foreach { case (host, port) =>
val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
// for some reason, the proxy can not handle redirect well, as a workaround,
// we simulate the Spark UI redirection behavior and forcibly rewrite the
// empty URI to the Spark Jobs page.
case "" | "/" => "/jobs/"
- case path => path
+ case path => path + queryString
Review Comment:
jobs also need queryString
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] codecov-commenter commented on pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1600012322
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/4983?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#4983](https://app.codecov.io/gh/apache/kyuubi/pull/4983?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (53f1ce4) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/34bb1d4dbe76e8719590e84482078d94f892f522?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (34bb1d4) will **not change** coverage.
> The diff coverage is `0.00%`.
> :exclamation: Current head 53f1ce4 differs from pull request most recent head 892c551. Consider uploading reports for the commit 892c551 to get more accurate results
```diff
@@ Coverage Diff @@
## master #4983 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 561 561
Lines 30956 30964 +8
Branches 4060 4061 +1
======================================
- Misses 30956 30964 +8
```
| [Impacted Files](https://app.codecov.io/gh/apache/kyuubi/pull/4983?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...pache/kyuubi/server/api/EngineUIProxyServlet.scala](https://app.codecov.io/gh/apache/kyuubi/pull/4983?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL0VuZ2luZVVJUHJveHlTZXJ2bGV0LnNjYWxh) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1234981395
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -30,14 +30,15 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
+ val queryString = getQueryString(request)
var targetURL = "/no-ui-error"
extractTargetAddress(requestURI).foreach { case (host, port) =>
val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
// for some reason, the proxy can not handle redirect well, as a workaround,
// we simulate the Spark UI redirection behavior and forcibly rewrite the
// empty URI to the Spark Jobs page.
case "" | "/" => "/jobs/"
- case path => path
+ case path => path + queryString
Review Comment:
OK. will add.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1600210821
> > close #4982 The targetURL already got the query string and kyuubi server can forward request to right page, eg: /engine-ui/spark-43efae88d766226a-driver-svc.kyuubi-dlssjc-canned.svc:4045/jobs/job/?id=4
>
> Please use the details template provided by Kyuubi.
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1598412986
> > close #4982 The targetURL already got the query string and kyuubi server can forward request to right page, eg: /engine-ui/spark-43efae88d766226a-driver-svc.kyuubi-dlssjc-canned.svc:4045/jobs/job/?id=4
>
> Please use the details template provided by Kyuubi.
got it. Thanks.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4983: [KYUUBI #4982][UI] Add query string to forward requests to Engine UI
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1605891167
Thanks, merged to master
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on pull request #4983: [KYUUBI #4982][UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1605890590
Perfect !! @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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1236698472
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -30,14 +30,15 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
+ val queryString = getQueryString(request)
var targetURL = "/no-ui-error"
extractTargetAddress(requestURI).foreach { case (host, port) =>
val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
// for some reason, the proxy can not handle redirect well, as a workaround,
// we simulate the Spark UI redirection behavior and forcibly rewrite the
// empty URI to the Spark Jobs page.
case "" | "/" => "/jobs/"
- case path => path
+ case path => path + queryString
Review Comment:
Done. Thanks.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zwangsheng commented on pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1598326599
> close #4982 The targetURL already got the query string and kyuubi server can forward request to right page, eg: /engine-ui/spark-43efae88d766226a-driver-svc.kyuubi-dlssjc-canned.svc:4045/jobs/job/?id=4
Please use the details template provided by Kyuubi.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#issuecomment-1598309042
@pan3793 @zwangsheng plz help review this pr
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 closed pull request #4983: [KYUUBI #4982][UI] Add query string to forward requests to Engine UI
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #4983: [KYUUBI #4982][UI] Add query string to forward requests to Engine UI
URL: https://github.com/apache/kyuubi/pull/4983
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] cxzl25 commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1239481609
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -62,4 +64,16 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ def getQueryString(request: HttpServletRequest): String = {
+ val result = new StringBuilder()
+ val queryString = request.getQueryString()
+ if (queryString != null && queryString.length() > 0) {
+ info(queryString)
+ result.append("?")
+ result.append(queryString)
+ }
+ result.toString()
+ }
+
}
Review Comment:
```suggestion
def getQueryString(request: HttpServletRequest): String = {
val queryString = request.getQueryString
if (StringUtils.isNotEmpty(queryString)) s"?$queryString" else ""
}
}
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zwangsheng commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zwangsheng (via GitHub)" <gi...@apache.org>.
zwangsheng commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1236418792
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -30,14 +30,15 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
+ val queryString = getQueryString(request)
var targetURL = "/no-ui-error"
extractTargetAddress(requestURI).foreach { case (host, port) =>
val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
// for some reason, the proxy can not handle redirect well, as a workaround,
// we simulate the Spark UI redirection behavior and forcibly rewrite the
// empty URI to the Spark Jobs page.
case "" | "/" => "/jobs/"
- case path => path
+ case path => path + queryString
Review Comment:
We can move those `append logic` out. Like:
``` scala
val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
case "" | "/" => "/jobs/"
case path => path
} + queryString
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] zhaohehuhu commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "zhaohehuhu (via GitHub)" <gi...@apache.org>.
zhaohehuhu commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1236409316
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -30,14 +30,15 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
override def rewriteTarget(request: HttpServletRequest): String = {
val requestURL = request.getRequestURL
val requestURI = request.getRequestURI
+ val queryString = getQueryString(request)
var targetURL = "/no-ui-error"
extractTargetAddress(requestURI).foreach { case (host, port) =>
val targetURI = requestURI.stripPrefix(s"/engine-ui/$host:$port") match {
// for some reason, the proxy can not handle redirect well, as a workaround,
// we simulate the Spark UI redirection behavior and forcibly rewrite the
// empty URI to the Spark Jobs page.
case "" | "/" => "/jobs/"
- case path => path
+ case path => path + queryString
Review Comment:
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] cxzl25 commented on a diff in pull request #4983: [Improvement] [UI] Add query string to forward requests to Engine UI
Posted by "cxzl25 (via GitHub)" <gi...@apache.org>.
cxzl25 commented on code in PR #4983:
URL: https://github.com/apache/kyuubi/pull/4983#discussion_r1239481609
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/api/EngineUIProxyServlet.scala:
##########
@@ -62,4 +64,16 @@ private[api] class EngineUIProxyServlet extends ProxyServlet with Logging {
case r(host, port) => Some(host -> port.toInt)
case _ => None
}
+
+ def getQueryString(request: HttpServletRequest): String = {
+ val result = new StringBuilder()
+ val queryString = request.getQueryString()
+ if (queryString != null && queryString.length() > 0) {
+ info(queryString)
+ result.append("?")
+ result.append(queryString)
+ }
+ result.toString()
+ }
+
}
Review Comment:
```suggestion
def getQueryString(request: HttpServletRequest): String = {
val queryString = request.getQueryString
if (StringUtils.isNotEmpty(queryString)) {
s"?$queryString"
} else {
""
}
}
}
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org