Skip to content
This repository was archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-1191] Allow specification of log directories in job parameters#877

Closed
afchung wants to merge 1 commit intoapache:masterfrom
afchung:REEF-1191
Closed

[REEF-1191] Allow specification of log directories in job parameters#877
afchung wants to merge 1 commit intoapache:masterfrom
afchung:REEF-1191

Conversation

@afchung
Copy link
Copy Markdown
Contributor

@afchung afchung commented Mar 7, 2016

JIRA:
[REEF-1191] https://issues.apache.org/jira/browse/REEF-1191

Pull Request:
Closes #


/// <summary>
/// Gets the file path for stdout for the driver.
/// </summary>
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.

Could you add the same description here, too?

/// Certain runtimes may impose stricter policies on log file
/// output locations (e.g. YARN jobs have restricted write access
/// to certain folders).

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.

Whoops, sorry, I meant to remove that comment entirely, because it turned out not to be true -- I was just entering the paths incorrectly when I tried to test it out. I'll remove the message in the other places. Thanks!

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.

:)

@dongjoon-hyun
Copy link
Copy Markdown
Member

Oh, @afchung . It fails on CI. Let's continue after fixing that first. :)

@afchung
Copy link
Copy Markdown
Contributor Author

afchung commented Mar 7, 2016

@dongjoon-hyun Sorry, I only ran .NET tests prior to sending the PR. Let me fix the tests and close this first in the meantime. Thanks for the review!

@afchung afchung closed this Mar 7, 2016
@afchung afchung reopened this Mar 7, 2016
@afchung
Copy link
Copy Markdown
Contributor Author

afchung commented Mar 7, 2016

@dongjoon-hyun I've updated the PR, addressing your comments and fixing the broken tests. Please have another look. Thanks!

@dongjoon-hyun
Copy link
Copy Markdown
Member

Thank you for quick fixing, @afchung .

@dongjoon-hyun
Copy link
Copy Markdown
Member

Sorry, @afchung .
The code looks good to me overall, but I couldn't pass the test due to the failure I wrote in dev mailing list.

I'm afraid I can't fix my build environment soon.
I hope some other committer help to finish to review and commit this.
Could you ask someone else explicitly, please?

@markusweimer
Copy link
Copy Markdown
Contributor

@afchung Can you please resolve the merge conflicts here?

@afchung
Copy link
Copy Markdown
Contributor Author

afchung commented Mar 9, 2016

@markusweimer Resolved, thanks!

@jwang98052
Copy link
Copy Markdown
Contributor

Do you have a test case for the change especially test running on YARN?

@afchung
Copy link
Copy Markdown
Contributor Author

afchung commented Mar 21, 2016

@jwang98052 I'm not sure how you would test this on YARN, since the log location only specifies where the log lives and has no verifiable property in the job itself.

@afchung
Copy link
Copy Markdown
Contributor Author

afchung commented Mar 24, 2016

@jwang98052 Ping...

@jwang98052
Copy link
Copy Markdown
Contributor

@afchung Could you please rebase again?

@afchung
Copy link
Copy Markdown
Contributor Author

afchung commented Mar 29, 2016

@jwang98052 rebased.

@jwang98052
Copy link
Copy Markdown
Contributor

I will test and merge

@asfgit asfgit closed this in 1f965d7 Mar 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants