Skip to content

Fixed a typo in DAGScheduler.#8308

Closed
zzvara wants to merge 2 commits into
apache:masterfrom
zzvara:master
Closed

Fixed a typo in DAGScheduler.#8308
zzvara wants to merge 2 commits into
apache:masterfrom
zzvara:master

Conversation

@zzvara

@zzvara zzvara commented Aug 19, 2015

Copy link
Copy Markdown
Contributor

No description provided.

@sarutak

sarutak commented Aug 19, 2015

Copy link
Copy Markdown
Member

ok to test.

@srowen

srowen commented Aug 19, 2015

Copy link
Copy Markdown
Member

Yeah, pretty trivial but true. Same for taskSetFailed I think.
Also executorLost and executorAdded could probably refer to a "TaskScheduler implementation" to be more accurate.

@zzvara

zzvara commented Aug 19, 2015

Copy link
Copy Markdown
Contributor Author

Would you like if I go through all of them?
Okay, I will.

@srowen

srowen commented Aug 19, 2015

Copy link
Copy Markdown
Member

I think the additional changes I mentioned are worth fixing here since they're very much of the same form. If you have time to scan for similar problems in nearby source, that's fine, but fixing all the related issues in this file seems like a good logical change.

@SparkQA

SparkQA commented Aug 19, 2015

Copy link
Copy Markdown

Test build #41234 has finished for PR 8308 at commit 2a70faf.

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

@zzvara

zzvara commented Aug 20, 2015

Copy link
Copy Markdown
Contributor Author

I went through it, also reorganized a method to the front and made method comments consistent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure all of this should be scaladoc. This looks like a private comment in the source code. Same for "Broken out for easier testing..."

@SparkQA

SparkQA commented Aug 20, 2015

Copy link
Copy Markdown

Test build #41325 has finished for PR 8308 at commit 4911559.

  • This patch fails to build.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why was this changed? it's not a big deal, but it was OK on one line. Changes always have this small but non-zero chance of tangling up a merge later, so trivial changes are usually avoided

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess it must have been going over 100 in length.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh right, because of the extra word. That's fine.
PS You'll need to rebase this branch on master

@SparkQA

SparkQA commented Aug 20, 2015

Copy link
Copy Markdown

Test build #41326 has finished for PR 8308 at commit 100f9a7.

  • This patch fails to build.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Aug 24, 2015

Copy link
Copy Markdown

Test build #41450 timed out for PR 8308 at commit f835ae8 after a configured wait of 175m.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changes here that just make scaladoc into a multiline comment don't do anything .. it's still valid on one line. I think the other changes look fine, and this is a no-op at best, but personally would not change these lines if there is no functional change.

@srowen

srowen commented Aug 25, 2015

Copy link
Copy Markdown
Member

LGTM

@SparkQA

SparkQA commented Aug 25, 2015

Copy link
Copy Markdown

Test build #41529 has finished for PR 8308 at commit 73ce4ac.

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

@srowen

srowen commented Aug 25, 2015

Copy link
Copy Markdown
Member

The test failure must be spurious; this is a scaladoc-only change, and compilation / style checks succeed.

asfgit pushed a commit that referenced this pull request Aug 25, 2015
Author: ehnalis <zoltan.zvara@gmail.com>

Closes #8308 from ehnalis/master.

(cherry picked from commit 7f1e507)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@asfgit asfgit closed this in 7f1e507 Aug 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants