Add "open" method #1895

arielshaqed · 2021-05-05T14:00:14Z

Untested -- need an integration test (vs. e.g. minIO and/or true S3).

Closes #1856.

arielshaqed · 2021-05-05T14:05:22Z

Manual testing: Start a Spark shell configured to run everything:

spark-shell  --packages org.apache.hadoop:hadoop-aws:2.7.7  --jars ./target/hadoopfs-0.1.0.jar --master local  --conf=spark.hadoop.fs.lakefs.impl=io.lakefs.LakeFSFileSystem  --conf=spark.driver.extraJavaOptions='-Dcom.amazonaws.services.s3.enableV4'      --conf=spark.hadoop.lakefs.api.url=http://localhost:8000/api/v1  --conf=spark.hadoop.fs.lakefs.access.key=AKIAlakeFS --conf=spark.hadoop.fs.lakefs.secret.key=lakeFShhhh --conf=spark.hadoop.fs.s3a.access.key=AKIApleaseDONTstealMYawsKEY --conf=spark.hadoop.fs.s3a.secret.key=Shhh...  --conf=spark.hadoop.fs.s3a.endpoint=s3.eu-central-1.amazonaws.com

Now run this in the shell:

scala> import org.apache.hadoop.fs.Path; val p = new Path("lakefs://moo/main/yes"); val fs = p.getFileSystem(sc.hadoopConfiguration);      val c = fs.open(p)
import org.apache.hadoop.fs.Path
p: org.apache.hadoop.fs.Path = lakefs://moo/main/yes
fs: org.apache.hadoop.fs.FileSystem = io.lakefs.LakeFSFileSystem@3b59d246
c: org.apache.hadoop.fs.FSDataInputStream = org.apache.hadoop.fs.FSDataInputStream@3fbf4614

scala> val buf = new Array[Byte](5555)
buf: Array[Byte] = Array(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,...

scala> val len = c.read(buf)
len: Int = 5

scala> new String(buf.slice(0, len))
res0: String =
"huh!
"

arielshaqed · 2021-05-05T14:08:38Z

[+ @nopcoder for general layout, try to share translateUri, etc.]

nopcoder · 2021-05-05T17:08:04Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSFileSystem.java

-    private io.lakefs.clients.api.ApiClient apiClient;
+    private ApiClient apiClient;
+
+    private static class ParsedPath {


made something like that 😃

think there is indent issue started after this line

nopcoder · 2021-05-05T22:00:22Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSFileSystem.java

+	if (!parts[0].equals("")) {
+	    throw new RuntimeException("path does not start with / (" + String.join(" [/] ", parts) + ")");
+	}
+	return new ParsedPath(pathUri.getAuthority(), parts[1], parts[2]);


used the getHost for this one as we don't use the port for the repository - not sure which one is the better

nopcoder · 2021-05-05T22:05:06Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSFileSystem.java

+	    URI physicalUri = translateUri(new URI(stats.getPhysicalAddress()));
+
+	    Path physicalPath = new Path(physicalUri.toString());
+	    FileSystem physicalFs = physicalPath.getFileSystem(conf);


This is the part where I thought we need to initialize the underlying filesystem once in the initialize and use it here and just call open.

Yeah, I was hoping to do that too. Disappointed that this is actually surprisingly difficult to do correctly. Now according to S3AFileSystem.initialize (please look at the code), the repository is guaranteed to exist on the URL. (TBH, I don't understand why, but we might assume as much). So we could generate the underlying file system at that point.

Let's circle back to this point once we have a working FS? Because what this would do is (only) speed up multiple calls to open (and create, after #1858 ) when they occur on the same FileSystem. I.e. it speeds up the already-fast part of multi-part file read/write operations -- if it works.

Opened #1906.

nopcoder · 2021-05-05T22:05:51Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSFileSystem.java

+	    FileSystem physicalFs = physicalPath.getFileSystem(conf);
+	    return physicalFs.open(physicalPath, bufSize);
+	} catch (io.lakefs.clients.api.ApiException e) {
+	    throw new RuntimeException("API exception: " + e.getResponseBody());


think you can pass e as inner exception here

Yup, forgot about exception causes (aka "Go %w done right"...). Thanks!

talSofer

@arielshaqed I reviewed this, and currently don't have comments to add to Barak's comments

arielshaqed

Thanks!

PTAL...

arielshaqed · 2021-05-06T14:55:01Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSFileSystem.java

+	    URI physicalUri = translateUri(new URI(stats.getPhysicalAddress()));
+
+	    Path physicalPath = new Path(physicalUri.toString());
+	    FileSystem physicalFs = physicalPath.getFileSystem(conf);


Yeah, I was hoping to do that too. Disappointed that this is actually surprisingly difficult to do correctly. Now according to S3AFileSystem.initialize (please look at the code), the repository is guaranteed to exist on the URL. (TBH, I don't understand why, but we might assume as much). So we could generate the underlying file system at that point.

Let's circle back to this point once we have a working FS? Because what this would do is (only) speed up multiple calls to open (and create, after #1858 ) when they occur on the same FileSystem. I.e. it speeds up the already-fast part of multi-part file read/write operations -- if it works.

Opened #1906.

arielshaqed · 2021-05-06T14:56:27Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSFileSystem.java

+	    FileSystem physicalFs = physicalPath.getFileSystem(conf);
+	    return physicalFs.open(physicalPath, bufSize);
+	} catch (io.lakefs.clients.api.ApiException e) {
+	    throw new RuntimeException("API exception: " + e.getResponseBody());


Yup, forgot about exception causes (aka "Go %w done right"...). Thanks!

Untested -- need an integration test (vs. e.g. minIO and/or true S3).

(Was concurrently added to our sources, 1 implementation is better than 2)

arielshaqed · 2021-05-09T07:35:22Z

Thanks!

Pulling...

arielshaqed requested a review from talSofer May 5, 2021 14:00

arielshaqed force-pushed the feature/1856-thick-open branch from 813b1d0 to a7783a6 Compare May 5, 2021 14:01

arielshaqed requested a review from nopcoder May 5, 2021 14:08

nopcoder reviewed May 5, 2021

View reviewed changes

talSofer reviewed May 6, 2021

View reviewed changes

arielshaqed force-pushed the feature/1856-thick-open branch from a7783a6 to d3da830 Compare May 6, 2021 14:25

arielshaqed commented May 6, 2021

View reviewed changes

arielshaqed added 3 commits May 7, 2021 17:51

Add "open" method

363c0a9

Untested -- need an integration test (vs. e.g. minIO and/or true S3).

[CR] Replace ParsedPath with ObjectLocation

9b35cb1

(Was concurrently added to our sources, 1 implementation is better than 2)

[CR] Rethrow lakeFS API exception as causes

340e7b2

arielshaqed force-pushed the feature/1856-thick-open branch from 257f9b5 to 340e7b2 Compare May 7, 2021 14:51

talSofer approved these changes May 9, 2021

View reviewed changes

arielshaqed merged commit 5996edf into master May 9, 2021

arielshaqed deleted the feature/1856-thick-open branch May 9, 2021 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "open" method #1895

Add "open" method #1895

arielshaqed commented May 5, 2021

arielshaqed commented May 5, 2021

arielshaqed commented May 5, 2021

nopcoder May 5, 2021

nopcoder May 5, 2021

nopcoder May 5, 2021

arielshaqed May 6, 2021

nopcoder May 5, 2021

arielshaqed May 6, 2021

talSofer left a comment

arielshaqed left a comment

arielshaqed May 6, 2021

arielshaqed May 6, 2021

arielshaqed commented May 9, 2021

Add "open" method #1895

Add "open" method #1895

Conversation

arielshaqed commented May 5, 2021

arielshaqed commented May 5, 2021

arielshaqed commented May 5, 2021

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

talSofer left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed commented May 9, 2021