Skip to content

Conversation

@chutium
Copy link
Contributor

@chutium chutium commented Nov 30, 2015

EventLoggingListener.getLogPath needs 4 input arguments:
https://github.com/apache/spark/blob/v1.6.0-preview2/core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala#L276-L280

the 3rd parameter should be appAttemptId, 4th parameter is codec...

@vanzin
Copy link
Contributor

vanzin commented Nov 30, 2015

Ah, good catch. While you're there, can you change the fourth parameter to explicitly have a name? e.g.:

getLogPath(foo, bar, baz, compressionCodecName = codec)

There should have been a warning on the output about that missing name, although it's pretty easy to ignore warnings... :-/

@chutium
Copy link
Contributor Author

chutium commented Nov 30, 2015

ha, yes, it happens, okey, i will add some parameter names

@andrewor14
Copy link
Contributor

LGTM good catch

@SparkQA
Copy link

SparkQA commented Nov 30, 2015

Test build #46897 has finished for PR 10044 at commit c9b20f9.

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

@SparkQA
Copy link

SparkQA commented Nov 30, 2015

Test build #46900 has finished for PR 10044 at commit 0b0a053.

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

@sarutak
Copy link
Member

sarutak commented Nov 30, 2015

LGTM, merging into master, branch-1.6, branch-1.5 and branch-1.4. Thanks @chutium !

asfgit pushed a commit that referenced this pull request Nov 30, 2015
```EventLoggingListener.getLogPath``` needs 4 input arguments:
https://github.com/apache/spark/blob/v1.6.0-preview2/core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala#L276-L280

the 3rd parameter should be appAttemptId, 4th parameter is codec...

Author: Teng Qiu <[email protected]>

Closes #10044 from chutium/SPARK-12053.

(cherry picked from commit a8ceec5)
Signed-off-by: Kousuke Saruta <[email protected]>
asfgit pushed a commit that referenced this pull request Nov 30, 2015
```EventLoggingListener.getLogPath``` needs 4 input arguments:
https://github.com/apache/spark/blob/v1.6.0-preview2/core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala#L276-L280

the 3rd parameter should be appAttemptId, 4th parameter is codec...

Author: Teng Qiu <[email protected]>

Closes #10044 from chutium/SPARK-12053.

(cherry picked from commit a8ceec5)
Signed-off-by: Kousuke Saruta <[email protected]>
@asfgit asfgit closed this in a8ceec5 Nov 30, 2015
@sarutak
Copy link
Member

sarutak commented Nov 30, 2015

I noticed we cannot merge this patch without modifying manually so I don't merge this into branch-1.4 for now. Please feel free to open another PR for branch-1.4.

@chutium
Copy link
Contributor Author

chutium commented Dec 1, 2015

thanks for merging, i will check this for branch-1.4

@chutium
Copy link
Contributor Author

chutium commented Dec 1, 2015

@sarutak it seems branch-1.4 was correct with EventLoggingListener.getLogPath
https://github.com/apache/spark/blob/v1.4.1/core/src/main/scala/org/apache/spark/deploy/master/Master.scala#L770-L771

the 3rd parameter None in this part of code was somehow missing after 1.5 ... :-o

@chutium
Copy link
Contributor Author

chutium commented Dec 1, 2015

it was caused by this commit about half year ago: 3bee0f1#diff-29dffdccd5a7f4c8b496c293e87c8668L771

hope no other small hidden bugs there... maybe worth a double check again

@sarutak
Copy link
Member

sarutak commented Dec 1, 2015

Yeah, I also noticed that branch-1.4 has no issue discussed in this PR.

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.

5 participants