FTP - toPath sink #182 #184

svezfaz · 2017-02-13T08:29:49Z

Handles #182

eharik · 2017-02-13T12:54:59Z

ftp/src/test/scala/akka/stream/alpakka/ftp/CommonFtpStageSpec.scala

@@ -97,4 +99,47 @@ trait CommonFtpSourceSpec extends BaseSpec {
    }
  }

+  "FtpIOSink" should {


is it worth having a spec showing the "sad path?" maybe an IOException when trying to write to a location without sufficient permissions?

thanks for reviewing, that's a fair point

maybe I could leave the anon user with writepermissions=false for the sad path + setup a different, write-enabled user for the happy path?

@svezfaz That would be great!.

I can easily manage FtpServer permissions with the users.properties file, but still haven't worked out how to do it for Sshd/Jimfs (SFTP tests).

I haven't done yet either. It seems that you can control the permissions set with chmod.

@juanjovazquez @eharik I tried several approaches, chmod being one of them. But ultimately, the underlying filesystem is Jimfs - which does not support posix-like permissions.

For example, while setting and reading POSIX file permissions is possible with the "posix" view, those permissions will not actually affect the behavior of the file system.

I don't see an easy way of testing this without moving to - say - Memoryfilesystem.
The good news is that - somehow obviously - when trying it with my local filesystem, I get IOExceptions for permission errors.

WDYT?

I don't know what Akka members team will think, but seeing that it is so difficult to test this particular case, I would let it be. I'd stick to jimfs as it's been very rewarding up until now.

sounds good to keep it simple, and stick to jimfs

juanjovazquez

Aside from these nitpicks, the PR LGTM. Great contribution!. I love dualities ;-).

juanjovazquez · 2017-02-13T15:50:28Z

ftp/src/main/scala/akka/stream/alpakka/ftp/impl/FtpIOGraphStage.scala

-import akka.stream.stage.{GraphStageWithMaterializedValue, OutHandler}
-import akka.stream.{Attributes, IOResult, Outlet, SourceShape}
+import akka.stream.stage.{GraphStageWithMaterializedValue, InHandler, OutHandler}
+import akka.stream._


I'd go with single type imports instead of on-demand style with the underscore. I'm not sure if there's a general policy for this.

juanjovazquez · 2017-02-13T15:51:16Z

ftp/src/main/scala/akka/stream/alpakka/ftp/impl/FtpIOGraphStage.scala

 import akka.stream.impl.Stages.DefaultAttributes.IODispatcher
 import akka.util.ByteString
 import akka.util.ByteString.ByteString1C
+


Remove this blank line?.

juanjovazquez · 2017-02-13T15:55:20Z

ftp/src/main/scala/akka/stream/alpakka/ftp/impl/FtpIOGraphStage.scala

 import java.nio.file.Path

-private[ftp] trait FtpIOGraphStage[FtpClient, S <: RemoteFileSettings]
-    extends GraphStageWithMaterializedValue[SourceShape[ByteString], Future[IOResult]] {
+private[ftp] trait FtpIOGraphStage[FtpClient, S <: RemoteFileSettings, Sh <: Shape]


Great idea to abstract over Shape. ;-)

juanjovazquez · 2017-02-13T16:07:55Z

ftp/src/test/scala/akka/stream/alpakka/ftp/CommonFtpStageSpec.scala

@@ -97,4 +99,47 @@ trait CommonFtpSourceSpec extends BaseSpec {
    }
  }

+  "FtpIOSink" should {


@svezfaz That would be great!.

juanjovazquez · 2017-02-13T16:16:41Z

BTW, It seems that #185 is still happening. I'm not able to reproduce it on my side. Maybe we should relax the PatienceConfig a bit more on CommonFtpSourceSpec trait. Maybe something like this?:

implicit val defaultPatience =
    PatienceConfig(timeout = Span(20, Seconds), interval = Span(900, Millis))

svezfaz · 2017-02-13T22:23:03Z

@juanjovazquez I have mixed-in IntegrationPatience to the relevant test to address the flaky tests.

johanandren

Added some feedback&questions.

johanandren · 2017-02-21T15:44:14Z

ftp/src/main/scala/akka/stream/alpakka/ftp/impl/FtpIOGraphStage.scala

+      protected[this] def matFailure(t: Throwable): Boolean =
+        matValuePromise.trySuccess(IOResult.createFailed(writtenBytesTotal, t))
+
+      /** BLOCKING I/O WRITE */


So, it already runs on a separate dispatcher from the stage superclass right?

johanandren · 2017-02-21T15:48:12Z

ftp/src/main/scala/akka/stream/alpakka/ftp/impl/FtpIOGraphStage.scala

+          pull(in)
+        } else
+          tryOs.failed.foreach { case NonFatal(t) => throw t }
+      }


This just mimics regular exception handling in a more complicated way. What about just calling get on the returned Try of storeFileStream, it will throw the exception if there is one and return the OutputStream if there is one.

johanandren · 2017-02-21T15:53:05Z

ftp/src/main/scala/akka/stream/alpakka/ftp/javadsl/FtpApi.scala

+   * @return A [[Sink]] of [[ByteString]] that materializes to a [[CompletionStage]] of [[IOResult]]
+   */
+  def toPath(
+      path: Path,


Is java.nio.Path really a good type to use here? The parameter refers to the path on the FTP server, while Path instances always has a FileSystem (which does not belong to the FTP server). Won't that mean that path.toAbsolutePath.toString likely will contain "C:\my\path" on windows for example?

I think it should just be a string, or a domain specific path-like type.

I think that makes sense. String path is also used in the ls Sinks already.

@juanjovazquez any particular reason why this was applied to fromPath ?

I can go ahead and use String in both fromPath and toPath.

@svezfaz IIRC that idea also came from Camel. But, I think @johanandren 's considerations make sense. I had that doubt during the first implementation and now I realize that I took the wrong path after all. So, @svezfaz go ahead with these changes if you are willing to. Thanks!.

johanandren · 2017-02-21T15:54:30Z

ftp/src/test/java/org/apache/ftpserver/filesystem/jimfs/impl/JimfsFtpFile.java

+                        PosixFilePermissions.fromString("rwxrwxrwx");
+                FileAttribute<Set<PosixFilePermission>> attr =
+                        PosixFilePermissions.asFileAttribute(perms);
+                Files.createDirectory(path, attr);


Are you sure this will only ever be called on a Posix/unixy file system?

This bit is actually only used in tests within a Unix-configured Jimfs instance. So my take is that it should be safe to do..

johanandren

LGTM

patriknw

LGTM

patriknw · 2017-03-23T11:18:43Z

ftp/src/test/scala/akka/stream/alpakka/ftp/CommonFtpStageSpec.scala

@@ -97,4 +99,47 @@ trait CommonFtpSourceSpec extends BaseSpec {
    }
  }

+  "FtpIOSink" should {


sounds good to keep it simple, and stick to jimfs

patriknw · 2017-03-23T11:21:37Z

@svezfaz @juanjovazquez anything more remaining here apart from a rebase?

svezfaz · 2017-03-23T11:28:49Z

@patriknw I think this one is ready to go. Further expansions will be handled in future tickets. Thanks for reviewing.

juanjovazquez · 2017-03-23T12:01:18Z

@svezfaz @patriknw LGTM

patriknw · 2017-03-23T15:40:36Z

thanks, please rebase @svezfaz

svezfaz · 2017-03-23T19:09:09Z

@patriknw rebased

eharik reviewed Feb 13, 2017

View reviewed changes

juanjovazquez reviewed Feb 13, 2017

View reviewed changes

svezfaz force-pushed the 182-FTP-toPath-sink branch from 6c4a544 to 4b5ba5f Compare February 13, 2017 22:12

svezfaz force-pushed the 182-FTP-toPath-sink branch 2 times, most recently from 6425a59 to cf97e05 Compare February 16, 2017 18:44

johanandren reviewed Feb 21, 2017

View reviewed changes

svezfaz force-pushed the 182-FTP-toPath-sink branch 2 times, most recently from 2e28cd6 to 4dbe282 Compare February 21, 2017 19:05

johanandren approved these changes Feb 22, 2017

View reviewed changes

patriknw self-assigned this Mar 20, 2017

patriknw approved these changes Mar 23, 2017

View reviewed changes

FTP - toPath sink akka#182

a265241

svezfaz force-pushed the 182-FTP-toPath-sink branch from 4dbe282 to a265241 Compare March 23, 2017 18:56

johanandren merged commit a38453c into akka:master Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FTP - toPath sink #182 #184

FTP - toPath sink #182 #184

svezfaz commented Feb 13, 2017

eharik Feb 13, 2017

svezfaz Feb 13, 2017

juanjovazquez Feb 13, 2017

svezfaz Feb 13, 2017 •

edited

Loading

juanjovazquez Feb 14, 2017

svezfaz Feb 16, 2017

juanjovazquez Feb 16, 2017

patriknw Mar 23, 2017

juanjovazquez left a comment

juanjovazquez Feb 13, 2017

juanjovazquez Feb 13, 2017

juanjovazquez Feb 13, 2017

juanjovazquez Feb 13, 2017

juanjovazquez commented Feb 13, 2017

svezfaz commented Feb 13, 2017

johanandren left a comment

johanandren Feb 21, 2017

svezfaz Feb 21, 2017

johanandren Feb 21, 2017

johanandren Feb 21, 2017

svezfaz Feb 21, 2017

juanjovazquez Feb 21, 2017

johanandren Feb 21, 2017

svezfaz Feb 21, 2017

johanandren left a comment

patriknw left a comment

patriknw Mar 23, 2017

patriknw commented Mar 23, 2017

svezfaz commented Mar 23, 2017

juanjovazquez commented Mar 23, 2017

patriknw commented Mar 23, 2017

svezfaz commented Mar 23, 2017

FTP - toPath sink #182 #184

FTP - toPath sink #182 #184

Conversation

svezfaz commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svezfaz Feb 13, 2017 • edited Loading

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

juanjovazquez left a 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

juanjovazquez commented Feb 13, 2017

svezfaz commented Feb 13, 2017

johanandren left a 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

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

johanandren left a comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw commented Mar 23, 2017

svezfaz commented Mar 23, 2017

juanjovazquez commented Mar 23, 2017

patriknw commented Mar 23, 2017

svezfaz commented Mar 23, 2017

svezfaz Feb 13, 2017 •

edited

Loading