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

[REEF-940] Create .NET job resource uploader using IFileSystem #637

Closed
wants to merge 5 commits into from

Conversation

anupam128
Copy link
Member

JIRA:
REEF-940


private long DateTimeToUnixTimestamp(DateTime dateTime)
{
return (long) (dateTime - Epoch).TotalSeconds;
Copy link
Contributor

Choose a reason for hiding this comment

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

UTC?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont know yet. Will fix when I figure out.

@afchung
Copy link
Contributor

afchung commented Nov 17, 2015

@anupam128 This looks good in general, just a few minor comments. Thanks!

@@ -59,6 +60,10 @@ under the License.
<Project>{5094c35b-4fdb-4322-ac05-45d684501cbf}</Project>
<Name>Org.Apache.REEF.Client</Name>
</ProjectReference>
<ProjectReference Include="..\Org.Apache.REEF.IO\Org.Apache.REEF.IO.csproj">
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually replace the .. with a variable.

@markusweimer
Copy link
Contributor

@anupam128 I've done a pass.

@anupam128
Copy link
Member Author

@afchung @markusweimer I have made the changes.

@afchung
Copy link
Contributor

afchung commented Nov 17, 2015

Looks good. Have you done any E2E tests?

@anupam128
Copy link
Member Author

@afchung I don't have a way to do E2E test yet (HadoopFileSystem does not have GetFileStatus API)

REEF-922 with include this E2E testing.

@afchung
Copy link
Contributor

afchung commented Nov 18, 2015

@anupam128 OK, I'll test and merge after @markusweimer signs off.

/// </summary>
internal sealed class FileSystemJobResourceUploader : IJobResourceUploader
{
private static readonly Logger Log = Logger.GetLogger(typeof(LegacyJobResourceUploader));
Copy link
Contributor

Choose a reason for hiding this comment

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

This uses the wrong logger name, I believe?

@markusweimer
Copy link
Contributor

I did another pass, left a comment. Other than that: +1 on merging.

@anupam128
Copy link
Member Author

@afchung @markusweimer thanks for reviewing. I have made the change.

@afchung
Copy link
Contributor

afchung commented Nov 18, 2015

I'll test and merge.

@asfgit asfgit closed this in b99b96a Nov 19, 2015
markusweimer pushed a commit to markusweimer/reef that referenced this pull request Dec 7, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants