Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SPARK-2168 [Spark core] Use relative URIs for the app links in the History Server. #1160

Closed
wants to merge 2 commits into from
Closed

SPARK-2168 [Spark core] Use relative URIs for the app links in the History Server. #1160

wants to merge 2 commits into from

Conversation

elyast
Copy link
Contributor

@elyast elyast commented Jun 20, 2014

Removing full URI leaving only relative path in link to the completed application plus unit test

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@vanzin
Copy link
Contributor

vanzin commented Jun 26, 2014

Haven't looked at the code yet, but could you add a more descriptive title to the PR? Thanks!

@vanzin
Copy link
Contributor

vanzin commented Jun 26, 2014

Also, this should probably go into master before it goes to branch-1.0.

@elyast elyast changed the title SPARK-2168 Spark core SPARK-2168 [Spark core] History Server renered page not suitable for load balancing, links to the completed applications should be relative Jun 26, 2014
@elyast
Copy link
Contributor Author

elyast commented Jun 26, 2014

I have added more descriptive title

@elyast
Copy link
Contributor Author

elyast commented Jun 26, 2014

so should I open another PR for master branch?

@vanzin
Copy link
Contributor

vanzin commented Jun 27, 2014

There's a typo in your title. :-)

Also, it could be shorter. Think that the PR title becomes the summary in the final git commit, so it shows in commands like "git log --oneline" where it's nice to have a short, concise description of the commit. I'd suggest something like this:

Use relative URIs for the app links in the History Server.

And then you can put a longer description in the description box, if you wish.

As for the branch, I'm not sure; that's beyond my github knowledge. I'd wait for a committer to chime in. :-)

@elyast elyast changed the title SPARK-2168 [Spark core] History Server renered page not suitable for load balancing, links to the completed applications should be relative SPARK-2168 [Spark core] History Server rendered page not suitable for load balancing, links to the completed applications should be relative Jun 27, 2014
@elyast elyast changed the title SPARK-2168 [Spark core] History Server rendered page not suitable for load balancing, links to the completed applications should be relative SPARK-2168 [Spark core] Use relative URIs for the app links in the History Server. Jul 1, 2014
@elyast
Copy link
Contributor Author

elyast commented Jul 1, 2014

I've shortened description thanks for hints.

@andrewor14
Copy link
Contributor

test this please

andrewor14
andrewor14 reviewed Aug 25, 2014
View reviewed changes
core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala
val expectedLink = response \\ "a"
expectedLink.size should equal(1)
val hrefAttr = expectedLink(0).attribute("href")
hrefAttr.get.toString should equal(link)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is weird here. Also, can you explain your comments "when" and "then"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the identation I was pretty sure it's fine when I was committing this one.

About "when" and "then", it's an old habit to simulate BDD-style of testing in Junit, however in Scala it could be replaced by using Specs2 or WordSpec trait from ScalaTest.

Let me know what u would like me to change.

@SparkQA
Copy link

SparkQA commented Sep 5, 2014

Can one of the admins verify this patch?

@andrewor14
Copy link
Contributor

ok to test...

@SparkQA
Copy link

SparkQA commented Sep 6, 2014

QA tests have started for PR 1160 at commit dc1f70a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 6, 2014

QA tests have finished for PR 1160 at commit dc1f70a.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

Hey @elyast could you open this against the master branch? It would be good if the latest Spark releases benefit from the changes here as well. For now, I would recommend that we close this PR in favor of one that updates the other branches.

@elyast
Copy link
Contributor Author

elyast commented Dec 24, 2014

@andrewor14 I think it's been fixed on master branch, so if you don't want to release maintenance release for 1.0.x then I would suggest to close it.

@andrewor14
Copy link
Contributor

I see. Nevertheless it would be good to merge the tests in master too. Feel free to open a new PR just for the test.

@andrewor14
Copy link
Contributor

Actually, let's retest this please

@SparkQA
Copy link

SparkQA commented Jan 7, 2015

Test build #25182 has started for PR 1160 at commit dc1f70a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 8, 2015

Test build #25182 has finished for PR 1160 at commit dc1f70a.

  • This patch fails some tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25182/
Test FAILed.

@andrewor14
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Jan 8, 2015

Test build #25194 has started for PR 1160 at commit dc1f70a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 8, 2015

Test build #25194 has finished for PR 1160 at commit dc1f70a.

  • This patch fails some tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25194/
Test FAILed.

@andrewor14
Copy link
Contributor

Looks like branch-1.0 is consistently failing tests (not related to this PR)

@srowen
Copy link
Member

srowen commented Feb 9, 2015

Is this still live? I think this PR should be closed in any event, but the test can go in master in a separate PR. Otherwise let's close this.

@elyast
Copy link
Contributor Author

elyast commented Feb 9, 2015

Fine with me, I will add tests on master with new PR

@elyast elyast closed this Feb 9, 2015
@elyast elyast mentioned this pull request Feb 26, 2015
Closed
asfgit pushed a commit that referenced this pull request Feb 27, 2015
@elyast
SPARK-2168 [Spark core] Use relative URIs for the app links in the Hi…
4a8a0a8 
…story Server.

As agreed in PR #1160 adding test to verify if history server generates relative links to applications.

Author: Lukasz Jastrzebski <[email protected]>

Closes #4778 from elyast/master and squashes the following commits:

0c07fab [Lukasz Jastrzebski] Incorporating comments for SPARK-2168
6d7866d [Lukasz Jastrzebski] Adjusting test for  SPARK-2168 for master branch
d6f4fbe [Lukasz Jastrzebski] Added test for  SPARK-2168
asfgit pushed a commit that referenced this pull request Feb 27, 2015
@elyast
SPARK-2168 [Spark core] Use relative URIs for the app links in the Hi…
485b919 
…story Server.

As agreed in PR #1160 adding test to verify if history server generates relative links to applications.

Author: Lukasz Jastrzebski <[email protected]>

Closes #4778 from elyast/master and squashes the following commits:

0c07fab [Lukasz Jastrzebski] Incorporating comments for SPARK-2168
6d7866d [Lukasz Jastrzebski] Adjusting test for  SPARK-2168 for master branch
d6f4fbe [Lukasz Jastrzebski] Added test for  SPARK-2168

(cherry picked from commit 4a8a0a8)
Signed-off-by: Andrew Or <[email protected]>
flyrain pushed a commit to flyrain/spark that referenced this pull request Sep 21, 2021
Branch 3.1.1 apple (apache#1160)
2eca24d 
* rdar://82095645 (Update the Callhome Version at Apple Spark Build) add jwt token on client side and send more fields to server for analytic and ML purpose
wangyum pushed a commit that referenced this pull request May 26, 2023
@allenma
[CARMEL-6399] Download hive-1.2.1 jar files to local to improve load …
f91eb96 
…properties performance (#1160)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@elyast @AmplabJenkins @vanzin @andrewor14 @SparkQA @srowen