You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "SteNicholas (via GitHub)" <gi...@apache.org> on 2023/10/13 06:11:16 UTC
[PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
SteNicholas opened a new pull request, #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986
### What changes were proposed in this pull request?
Improve response message of invalid HTTP request, which lists available API providers as below:
- master
```
Invalid uri of the master service. Available API providers includes:
/applications: List all running application's ids of the cluster.
/workerInfo: List worker information of the service. It will list all registered workers 's information.
/shuffles: List all running shuffle keys of the service. It will return all running shuffle's key of the cluster.
/threadDump: List the current thread dump of the master.
/hostnames: List all running application's LifecycleManager's hostnames of the cluster.
/masterGroupInfo: List master group information of the service. It will list all master's LEADER, FOLLOWER information.
/shutdownWorkers: List all shutdown workers of the master.
/lostWorkers: List all lost workers of the master.
/listTopDiskUsedApps: List the top disk usage application ids. It will return the top disk usage application ids for the cluster.
/conf: List the conf setting of the master.
/excludedWorkers: List all excluded workers of the master.
```
- worker
```
Invalid uri of the worker service. Available API providers includes:
/isRegistered: Show if the worker is registered to the master success.
/listPartitionLocationInfo: List all the living PartitionLocation information in that worker.
/workerInfo: List the worker information of the worker.
/shuffles: List all the running shuffle keys of the worker. It only return keys of shuffles running in that worker.
/threadDump: List the current thread dump of the worker.
/isShutdown: Show if the worker is during the process of shutdown.
/unavailablePeers: List the unavailable peers of the worker, this always means the worker connect to the peer failed.
/listTopDiskUsedApps: List the top disk usage application ids. It only return application ids running in that worker.
/conf: List the conf setting of the worker.
/exit: Trigger this worker to exit. Legal types are 'DECOMMISSION‘, 'GRACEFUL' and 'IMMEDIATELY'
```
### Why are the changes needed?
Response message of invalid HTTP request could not help users with correct HTTP path.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
`HttpUtilsSuite#CELEBORN-829: Improve response message of invalid HTTP request`
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #1986: [CELEBORN-829] Improve response message of invalid HTTP request
URL: https://github.com/apache/incubator-celeborn/pull/1986
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#issuecomment-1760970653
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1986](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6ad9709) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/c97628c510f7b8f0a9ca24c197679eb58e7f16f3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c97628c) will **decrease** coverage by `0.10%`.
> Report is 3 commits behind head on main.
> The diff coverage is `0.00%`.
> :exclamation: Current head 6ad9709 differs from pull request most recent head fb8cfe8. Consider uploading reports for the commit fb8cfe8 to get more accurate results
```diff
@@ Coverage Diff @@
## main #1986 +/- ##
==========================================
- Coverage 46.95% 46.85% -0.10%
==========================================
Files 164 165 +1
Lines 10380 10446 +66
Branches 959 948 -11
==========================================
+ Hits 4873 4893 +20
- Misses 5189 5236 +47
+ Partials 318 317 -1
```
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...leborn/server/common/http/HttpRequestHandler.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2NlbGVib3JuL3NlcnZlci9jb21tb24vaHR0cC9IdHRwUmVxdWVzdEhhbmRsZXIuc2NhbGE=) | `0.00% <0.00%> (ø)` | |
| [...apache/celeborn/server/common/http/HttpUtils.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2NlbGVib3JuL3NlcnZlci9jb21tb24vaHR0cC9IdHRwVXRpbHMuc2NhbGE=) | `0.00% <0.00%> (ø)` | |
| [...che/celeborn/server/common/http/HttpEndpoint.scala](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2NlbGVib3JuL3NlcnZlci9jb21tb24vaHR0cC9IdHRwRW5kcG9pbnQuc2NhbGE=) | `0.00% <0.00%> (ø)` | |
... and [4 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/1986/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#discussion_r1360204392
##########
service/src/main/scala/org/apache/celeborn/server/common/http/HttpEndpoint.scala:
##########
@@ -0,0 +1,218 @@
+/*
+ * 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.celeborn.server.common.http
+
+import org.apache.celeborn.server.common.{HttpService, Service}
+
+/**
+ * HTTP endpoints of Rest API providers.
+ */
+trait HttpEndpoint {
+ def path: String
+
+ def meaning(service: String): String
+
+ def handle(service: HttpService, parameters: Map[String, String]): String
+}
+
+case object Conf extends HttpEndpoint {
+ override def path: String = "/conf"
+
+ override def meaning(service: String): String = s"List the conf setting of the $service."
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getConf
+}
+
+case object WorkerInfo extends HttpEndpoint {
+ override def path: String = "/workerInfo"
+
+ override def meaning(service: String): String = {
+ if (service == Service.MASTER)
+ "List worker information of the service. It will list all registered workers 's information."
+ else "List the worker information of the worker."
+ }
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getWorkerInfo
+}
+
+case object ThreadDump extends HttpEndpoint {
+ override def path: String = "/threadDump"
+
+ override def meaning(service: String): String = s"List the current thread dump of the $service."
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getThreadDump
+}
+
+case object Shuffles extends HttpEndpoint {
+ override def path: String = "/shuffles"
+
+ override def meaning(service: String): String = {
+ if (service == Service.MASTER)
+ "List all running shuffle keys of the service. It will return all running shuffle's key of the cluster."
+ else
+ "List all the running shuffle keys of the worker. It only return keys of shuffles running in that worker."
+ }
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getShuffleList
+}
+
+case object ListTopDiskUsedApps extends HttpEndpoint {
+ override def path: String = "/listTopDiskUsedApps"
+
+ override def meaning(service: String): String = {
+ if (service == Service.MASTER)
+ "List the top disk usage application ids. It will return the top disk usage application ids for the cluster."
+ else
+ "List the top disk usage application ids. It only return application ids running in that worker."
+ }
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.listTopDiskUseApps
+}
+
+case object Help extends HttpEndpoint {
+ override def path: String = "/help"
+
+ override def meaning(service: String): String =
+ s"List the available API providers of the $service."
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ HttpUtils.help(service.serviceName)
+}
+
+case object Invalid extends HttpEndpoint {
+
+ val invalid = "invalid"
+
+ override def path: String = None.toString
Review Comment:
@cfmcgrady, there is no path previously for invalid which is `case _ => "invalid"`, therefore this uses `none` instead of `/invalid`.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#discussion_r1358021780
##########
service/src/main/scala/org/apache/celeborn/server/common/http/HttpUtils.scala:
##########
@@ -34,4 +55,29 @@ object HttpUtils {
}
(url.getPath, parameter)
}
+
+ def handleRequest(
+ service: HttpService,
+ path: String,
+ parameters: Map[String, String]): (String, Set[HttpEndpoint]) = {
+ var httpEndpoints: Set[HttpEndpoint] = null
+ if (service.serviceName == Service.MASTER) {
+ httpEndpoints = masterEndpoints
+ } else {
+ httpEndpoints = workerEndpoints;
+ }
+ (
+ httpEndpoints.find(endpoint => endpoint.path == path).orElse(Some(Invalid)).get.handle(
+ service,
+ parameters),
+ httpEndpoints)
+ }
+
+ def help(service: String, httpEndpoints: Set[HttpEndpoint]): String = {
+ val sb = new StringBuilder
+ sb.append(s"${Invalid.meaning(service)} Available API providers include:\n")
+ httpEndpoints.foreach(endpoint =>
+ sb.append(s"${endpoint.path}: ${endpoint.meaning(service)}\n"))
Review Comment:
` sb.append(s"${endpoint.path.padTo(30, "")} ${endpoint.meaning(service)}\n"))`
Can pad URI to a same length to make it more tidy.
Can the desc part of URI be made into a new line as many characters as possible?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#issuecomment-1761061641
@AngersZhuuuu, could you help to review this pull request? This pull request helps users with Rest API usage to get available API providers.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#discussion_r1360214025
##########
service/src/main/scala/org/apache/celeborn/server/common/http/HttpEndpoint.scala:
##########
@@ -0,0 +1,218 @@
+/*
+ * 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.celeborn.server.common.http
+
+import org.apache.celeborn.server.common.{HttpService, Service}
+
+/**
+ * HTTP endpoints of Rest API providers.
+ */
+trait HttpEndpoint {
+ def path: String
+
+ def meaning(service: String): String
Review Comment:
Will `description` be better?
##########
service/src/main/scala/org/apache/celeborn/server/common/http/HttpUtils.scala:
##########
@@ -20,8 +20,29 @@ package org.apache.celeborn.server.common.http
import java.net.URL
import java.util.Locale
+import org.apache.celeborn.server.common.{HttpService, Service}
+
object HttpUtils {
- def parseUrl(uri: String): (String, Map[String, String]) = {
+
+ private val baseEndpoints: List[HttpEndpoint] =
+ List(Conf, WorkerInfo, ThreadDump, Shuffles, ListTopDiskUsedApps, Help)
+ private val masterEndpoints: List[HttpEndpoint] = List(
+ MasterGroupInfo,
+ LostWorkers,
+ ExcludedWorkers,
+ ShutdownWorkers,
+ Hostnames,
+ Applications) ++ baseEndpoints
+ private val workerEndpoints: List[HttpEndpoint] =
+ List(
+ ListTopDiskUsedApps,
Review Comment:
`ListTopDiskUsedApps` is already in `baseEndpoints`
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#discussion_r1358157585
##########
docs/monitoring.md:
##########
@@ -301,6 +301,7 @@ API path listed as below:
| /applications | List all running application's ids of the cluster. |
| /shuffles | List all running shuffle keys of the service. It will return all running shuffle's key of the cluster. |
| /listTopDiskUsedApps | List the top disk usage application ids. It will return the top disk usage application ids for the cluster. |
+| /help | List the available API providers of the master. |
Review Comment:
Format here
##########
docs/monitoring.md:
##########
@@ -317,3 +318,4 @@ API path listed as below:
| /isShutdown | Show if the worker is during the process of shutdown. |
| /isRegistered | Show if the worker is registered to the master success. |
| /exit?type=${TYPE} | Trigger this worker to exit. Legal `type`s are 'DECOMMISSION‘, 'GRACEFUL' and 'IMMEDIATELY' |
+| /help | List the available API providers of the worker. |
Review Comment:
ditto
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "cfmcgrady (via GitHub)" <gi...@apache.org>.
cfmcgrady commented on code in PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#discussion_r1360182786
##########
service/src/main/scala/org/apache/celeborn/server/common/http/HttpEndpoint.scala:
##########
@@ -0,0 +1,218 @@
+/*
+ * 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.celeborn.server.common.http
+
+import org.apache.celeborn.server.common.{HttpService, Service}
+
+/**
+ * HTTP endpoints of Rest API providers.
+ */
+trait HttpEndpoint {
+ def path: String
+
+ def meaning(service: String): String
+
+ def handle(service: HttpService, parameters: Map[String, String]): String
+}
+
+case object Conf extends HttpEndpoint {
+ override def path: String = "/conf"
+
+ override def meaning(service: String): String = s"List the conf setting of the $service."
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getConf
+}
+
+case object WorkerInfo extends HttpEndpoint {
+ override def path: String = "/workerInfo"
+
+ override def meaning(service: String): String = {
+ if (service == Service.MASTER)
+ "List worker information of the service. It will list all registered workers 's information."
+ else "List the worker information of the worker."
+ }
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getWorkerInfo
+}
+
+case object ThreadDump extends HttpEndpoint {
+ override def path: String = "/threadDump"
+
+ override def meaning(service: String): String = s"List the current thread dump of the $service."
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getThreadDump
+}
+
+case object Shuffles extends HttpEndpoint {
+ override def path: String = "/shuffles"
+
+ override def meaning(service: String): String = {
+ if (service == Service.MASTER)
+ "List all running shuffle keys of the service. It will return all running shuffle's key of the cluster."
+ else
+ "List all the running shuffle keys of the worker. It only return keys of shuffles running in that worker."
+ }
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.getShuffleList
+}
+
+case object ListTopDiskUsedApps extends HttpEndpoint {
+ override def path: String = "/listTopDiskUsedApps"
+
+ override def meaning(service: String): String = {
+ if (service == Service.MASTER)
+ "List the top disk usage application ids. It will return the top disk usage application ids for the cluster."
+ else
+ "List the top disk usage application ids. It only return application ids running in that worker."
+ }
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ service.listTopDiskUseApps
+}
+
+case object Help extends HttpEndpoint {
+ override def path: String = "/help"
+
+ override def meaning(service: String): String =
+ s"List the available API providers of the $service."
+
+ override def handle(service: HttpService, parameters: Map[String, String]): String =
+ HttpUtils.help(service.serviceName)
+}
+
+case object Invalid extends HttpEndpoint {
+
+ val invalid = "invalid"
+
+ override def path: String = None.toString
Review Comment:
nit: may `/invalid` instead of `none`?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-829] Improve response message of invalid HTTP request [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #1986:
URL: https://github.com/apache/incubator-celeborn/pull/1986#issuecomment-1763993633
Thanks, merging to main/0.3
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org