[REEF-1339] Adding IInputPartition.Cache() for data download and cache #968

afchung · 2016-04-25T20:48:27Z

This addressed the issue by

Adding an unstable Cache function.
Modify existing implementations of IInputPartition to allow them to cache only on invocation of Cache instead of on initialization.

This addressed the issue by * Adding an unstable Cache function. * Modify existing implementations of IInputPartition to allow them to cache only on invocation of Cache instead of on initialization. JIRA: [REEF-1339](https://issues.apache.org/jira/browse/REEF-1339)

markusweimer · 2016-04-25T21:34:24Z

lang/cs/Org.Apache.REEF.IO/PartitionedData/FileSystem/FileSystemInputPartition.cs

}
+

Maybe move this into a method of its own? e.g. Download?

As per discussion below, for now, we will simply assume that Cache() in this class assumes a disk level cache.

I was more concerned about the relative length of this method. The if/else would be the only thing in this method, and the actual download logic would be in the helper methods:

if (!_localFiles.IsPresent()){ Download(); }

markusweimer · 2016-04-25T21:34:56Z

LGTM overall, with the one comment.

jwang98052 · 2016-04-25T22:02:56Z

I will have a look.

jwang98052 · 2016-04-25T22:53:06Z

lang/cs/Org.Apache.REEF.IO/PartitionedData/FileSystem/FileSystemInputPartition.cs

+
+            if (!copyToLocal)
+            {
+                // Implies that the files are already local.


copyToLocal means download from remote to local before reading the data.
!copyToLocal means read data from file (no matter files are on local or remote). It doesn't mean the file is already in local.

In our use case, the files are always remote.

For local test, the files can be at local, then copyToLocal is redundant, but still work.

I'm confused, then why is the code structured like this in the previous commit?

if (_copyToLocal) { if (!_isInitialized) { Initialize(); } return _fileSerializer.Deserialize(_localFiles); } return _fileSerializer.Deserialize(_filePaths);

@jwang98052 The previous code seems to imply that if not copy to local, then the files are already local.

In original code, files are provided in _filePaths, usually remote files.

If the user what to download files to local first, and if the files are not downloaded yet, then download (Initialize()), then Deserialize from the local downloaded files.

Else, meaning if user doesn't choose to copyToLocal, then Deserialize from the original files directly.

OK, and where are these "original files?" None of our IFileDeSerializer implementations provides the ability to deserialize remote files.

@afchung @jwang98052 my understanding is what andrew is saying. Even the code does that.

We have IFileDeSerializer implementation that read from remote file on the fly. Clients can choose to do it. The code in REEF doesn't show that sample, that's true.

dkm2110 · 2016-04-25T23:09:50Z

@afchung LGTM. I am wondering if Cache() should be added in DataLoadingContext in IMRU as part of this or later... Plus it would be good to check IMRU and make sure it did not break anything.

Once you are done with all reviews, ping me to check IMRU on cluster and I will. You can do it also :) if you want.

jwang98052 · 2016-04-26T00:20:05Z

We definitely want to test any changes in those classes in both local and Yarn. I have HadoopFileInputPartitionTest, you might want to update for Yarn testing.

When test it with IMRU, we need to set copytolocal both true and false to test it.

markusweimer · 2016-04-26T17:48:28Z

lang/cs/Org.Apache.REEF.IO/PartitionedData/FileSystem/FileSystemInputPartition.cs

        private readonly object _lock = new object();
        private readonly ITempFileCreator _tempFileCreator;
-        private readonly ISet<string> _localFiles = new HashSet<string>();
+        private readonly ISet<string> _filePaths;


Rename to _remoteFilePaths or such to make clear that these are remote.

In localRuntime or testing, it can be local files as well. In real use cases, yes, it represents remote files.

I think we should be explicit on what this is for. If this is only for remote files, we should make it so. For testing, we should fake the IFileSerializer to fetch from local instead of using an if branch the way we do right now that obfuscates the intention of the code.

Agree, it would be clear.

OK, for this PR, I will assume the following behavior:

IFileDeSerializer can be remote OR local.

_copyToLocal implies caching on access and that IFileDeSerializer is local. If _copyToLocal is false, we assume that IFileDeSerializer is remote.

We can clarify what IFileDeserializer is and what exactly _copyToLocal means in a separate JIRA item.

I've opened REEF-1365 for discussion.

afchung · 2016-04-26T23:58:54Z

@jwang98052 @markusweimer @dkm2110 I've addressed your comments and opened REEF-1339 for discussion, please have another look. Thanks!

jwang98052 · 2016-04-27T21:27:53Z

lang/cs/Org.Apache.REEF.IO/PartitionedData/FileSystem/FileSystemInputPartition.cs

+                    // For now, assume IFileDeSerializer is local.
+                    return _fileSerializer.Deserialize(_localFiles.Value);
+                }
+                else


no need else.

jwang98052 · 2016-04-27T21:30:35Z

LGTM with one minor comment.

afchung · 2016-04-27T22:40:23Z

@jwang98052 Addressed your comment.

jwang98052 · 2016-04-27T23:17:18Z

LGTM

markusweimer · 2016-04-28T16:47:10Z

I'll do a pass.

markusweimer · 2016-04-28T17:02:03Z

lang/cs/Org.Apache.REEF.IO/PartitionedData/IInputPartition.cs

    {
        /// <summary>
        /// The id of the partition.
        /// </summary>
        string Id { get; }

        /// <summary>
+        /// Caches the data locally, cached location is based on the implementation.
+        /// </summary>
+        [Unstable("0.14", "Contract may change.")]


Contract will change?

Why will? It won't necessarily change.

markusweimer · 2016-04-28T17:02:58Z

LGTM, I left some comments we may or may not address. Let me know whether you want to, @afchung. Otherwise, I will test and merge.

afchung · 2016-04-28T17:33:44Z

@markusweimer addressed your comments, please have another look. Thanks!

markusweimer · 2016-04-28T19:54:44Z

LGTM, will test and merge.

markusweimer reviewed Apr 25, 2016
View reviewed changes

jwang98052 reviewed Apr 25, 2016
View reviewed changes

markusweimer reviewed Apr 26, 2016
View reviewed changes

Addressed comments and improved code clarity.

c9d7bd0

jwang98052 reviewed Apr 27, 2016
View reviewed changes

Remove else.

a130d7a

markusweimer reviewed Apr 28, 2016
View reviewed changes

Address comments and remove redundant file checks.

a1dcc48

asfgit closed this in d34441c Apr 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REEF-1339] Adding IInputPartition.Cache() for data download and cache #968

[REEF-1339] Adding IInputPartition.Cache() for data download and cache #968

afchung commented Apr 25, 2016

markusweimer Apr 25, 2016

afchung Apr 26, 2016

markusweimer Apr 28, 2016

markusweimer commented Apr 25, 2016

jwang98052 commented Apr 25, 2016

jwang98052 Apr 25, 2016

afchung Apr 25, 2016 •

edited

afchung Apr 25, 2016

jwang98052 Apr 25, 2016

afchung Apr 25, 2016

dkm2110 Apr 25, 2016

jwang98052 Apr 26, 2016

dkm2110 commented Apr 25, 2016

jwang98052 commented Apr 26, 2016 •

edited

markusweimer Apr 26, 2016

jwang98052 Apr 26, 2016

afchung Apr 26, 2016

jwang98052 Apr 26, 2016

afchung Apr 26, 2016

afchung Apr 26, 2016

afchung commented Apr 26, 2016

jwang98052 Apr 27, 2016

jwang98052 commented Apr 27, 2016

afchung commented Apr 27, 2016

jwang98052 commented Apr 27, 2016

markusweimer commented Apr 28, 2016

markusweimer Apr 28, 2016

afchung Apr 28, 2016

markusweimer commented Apr 28, 2016

afchung commented Apr 28, 2016

markusweimer commented Apr 28, 2016

[REEF-1339] Adding IInputPartition.Cache() for data download and cache #968

[REEF-1339] Adding IInputPartition.Cache() for data download and cache #968

Conversation

afchung commented Apr 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusweimer commented Apr 25, 2016

jwang98052 commented Apr 25, 2016

Choose a reason for hiding this comment

afchung Apr 25, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkm2110 commented Apr 25, 2016

jwang98052 commented Apr 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afchung commented Apr 26, 2016

Choose a reason for hiding this comment

jwang98052 commented Apr 27, 2016

afchung commented Apr 27, 2016

jwang98052 commented Apr 27, 2016

markusweimer commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusweimer commented Apr 28, 2016

afchung commented Apr 28, 2016

markusweimer commented Apr 28, 2016

afchung Apr 25, 2016 •

edited

jwang98052 commented Apr 26, 2016 •

edited