You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by alexnaspo <gi...@git.apache.org> on 2017/10/17 18:58:46 UTC
[GitHub] spark pull request #19520: [SPARK-22298][WEB-UI]url encode API id before gen...
GitHub user alexnaspo opened a pull request:
https://github.com/apache/spark/pull/19520
[SPARK-22298][WEB-UI]url encode API id before generating rest endpoint for /allexecutors
## What changes were proposed in this pull request?
Spark Executor Page will return a blank list when the application id contains a forward slash. You can see the /allexecutors api failing with a 404. This can be fixed trivially by url encoding the appId before making the call to `/api/v1/applications/<appId>/allexecutors` in executorspage.js.
## How was this patch tested?
<img width="1035" alt="screen shot 2017-10-17 at 2 50 11 pm" src="https://user-images.githubusercontent.com/1855262/31683573-595e4be2-b34b-11e7-9974-3886fcf766fe.png">
<img width="1347" alt="screen shot 2017-10-17 at 2 57 36 pm" src="https://user-images.githubusercontent.com/1855262/31683659-90b0eae6-b34b-11e7-9018-5b271de9db28.png">
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alexnaspo/spark SPARK-22298-executors-url-encode
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19520.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19520
----
commit d4ce59685a3d44fbb46d8edfab9bc06e6dae1a41
Author: Alex Naspo <al...@gmail.com>
Date: 2017-10-17T18:53:50Z
url encode API id before generating rest endpoint for /allexecutors
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by ajbozarth <gi...@git.apache.org>.
Github user ajbozarth commented on the issue:
https://github.com/apache/spark/pull/19520
I'm ok either way but if we add this we should probably make sure we cover this issue anywhere else it may pop up
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19520
Which is not necessary if the cluster manager is doing the right thing, which it's not here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19520
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by ajbozarth <gi...@git.apache.org>.
Github user ajbozarth commented on the issue:
https://github.com/apache/spark/pull/19520
I'm a bit confused by the issue this is addressing, how do you get an appId with a `/` in it to beh=gin with, last I checked appId formats were hard coded inside Spark
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19520
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by alexnaspo <gi...@git.apache.org>.
Github user alexnaspo commented on the issue:
https://github.com/apache/spark/pull/19520
I agree that a / in the appId is not necessarily a good practice. However, would you think that the executor page should be resilient to this regardless of that fact?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by ajbozarth <gi...@git.apache.org>.
Github user ajbozarth commented on the issue:
https://github.com/apache/spark/pull/19520
I don't think this is something to "fix" in Spark, having slashes in the appId seems like a generally bad idea and this may not be the only place that it breaks things. Instead it may be better for Nomad to check appIds for slashes and reject them on creation.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19520
IMO non-URL friendly IDs (such as those containing slashes) also are not very user-friendly. So yes, this is a no-op for any case that we care about, but it's exposing what I consider a usability issue in some scheduler backend that has expressed interest in being added to Spark at some point.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19520
The app id is something that the cluster manager should be setting, not users, and it's pretty easy for cluster manager to generate Spark-friendly IDs. So not sure what problem this would be solving.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19520
We should close this. I don't see any user benefit in supporting slashes in app ids.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by alexnaspo <gi...@git.apache.org>.
Github user alexnaspo commented on the issue:
https://github.com/apache/spark/pull/19520
Yes, we can definitely remove it. However, it did take me a bit of time to determine why the executor page was broken. Was hoping to save this hassle for someone else in the future. It is unclear to me why spark would not URL encode.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/19520
Could you not do that instead? Or not use slashes?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by guoxiaolongzte <gi...@git.apache.org>.
Github user guoxiaolongzte commented on the issue:
https://github.com/apache/spark/pull/19520
I would like to ask, under what circumstances the application id will contain a forward slash?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19520: [SPARK-22298][WEB-UI] url encode APP id before ge...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19520
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by alexnaspo <gi...@git.apache.org>.
Github user alexnaspo commented on the issue:
https://github.com/apache/spark/pull/19520
The only goal is to to make the executor page resilient to this
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by alexnaspo <gi...@git.apache.org>.
Github user alexnaspo commented on the issue:
https://github.com/apache/spark/pull/19520
We set our own appId when running spark on Nomad. https://www.nomadproject.io/guides/spark/spark.html
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19520: [SPARK-22298][WEB-UI] url encode APP id before generatin...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19520
Should a cluster manager have to generate URL-friendly app IDs? I don't see a strong reason for that expectation. This doesn't affect any supported Spark integration, true. But this is basically a no-op in existing supported cases right? _shrug_ I'd probably just add this for future proofing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org