You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by eorgad <gi...@git.apache.org> on 2016/05/18 16:30:35 UTC
[GitHub] nifi pull request: Nifi 539c
GitHub user eorgad opened a pull request:
https://github.com/apache/nifi/pull/453
Nifi 539c
Commit review notes on GetSCP , PutSCP and SCPTransfer
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eorgad/nifi nifi-539c
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/453.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #453
----
commit 15b0321d3c4a69e778e4eebedc2dd1343ade8dd4
Author: eorgad <er...@gmail.com>
Date: 2016-05-18T16:26:40Z
Merge remote-tracking branch 'apache/master' into nifi-539c
commit e85ab80d50d9ce67905ea78e6386323cd42096e4
Author: eorgad <er...@gmail.com>
Date: 2016-05-18T16:29:26Z
GetSCP and PutSCP
Commit review notes on GetSCP , PutSCP and SCPTransfer
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220499039
@eorgad your interest and willingness to get involved with the NiFi community is certainly appreciated. In this case, I don't think the Travis build failed on the code in this PR. If you run "mvn clean install -Pcontrib-check" in your branch successfully, then that should give you confidence that your PR will be fine.
I do think there is value to a PutSCP processor over using ExecuteProcess. I personally wouldn't want to remember the specifics of the scp command in order to configure ExecuteProcess properly, so PutSCP would be a boon right there.
I think special care is needed to design this processor to the strengths and limitations of the scp protocol. For instance the PutSFTP processor will first sftp.put() to a temporary file, then sftp.rename() that temp file to the intended permanent name (this is to avoid the possibility that automated processes on the remote host pick up an incomplete file in the middle of transfer). PutSFTP also has options to sftp.setMtime(), sftp.chmod(), and sftp.chown(). I'm not sure how much of this interaction is supported in the scp protocol.
For processors like ListSCP or GetSCP, which probably would need to rely on getting a directory listing on the remote host, I'm having a difficult time determining if the scp protocol can do this. I can see a FetchSCP, though, when it's given a specific filename on the remote host to transfer into NiFi.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220173989
@mosermw I think as a community we should stay away from jumping to quick conclusions without giving this PR a thorough and adequate review as we do with others.
SFTP and SCP are two different protocols and while they both utilize the same SSH encryption during file transfer, there are quite a few differences which I am sure you can read up on, with performance being one of them (SCP being faster) etc.
As for JSch as a library, it is neither SFTP nor SCP. It is simple Java implementation of SSH2, allowing anyone to build components that need to interact over SSH. So _"processors here are still using SFTP under the covers"_ is not really accurate? What I think is more accurate is that both processors utilize the same library (JSch) to accomplish the set goal.
Now, I am not saying there are no issues with @eorgad's code, configuration, documentation etc. I have not personally had a chance to look through it yet, and I am sure there are things that may be incorrect or things that could be improved. So in the spirit of community and collaboration let's concentrate on those. There will be plenty of time in the end to cast up/down votes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220146745
It looks like GetSCP, PutSCP and SCPTransfer are just copies of the SFTP equivalent, with some cosmetic changes. They don't actually use scp. So I'm wondering where the added value is? We already have SFTP processors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220190814
For what it is worth @olegz I too prefer to see folks avoid using '-1' because it is a bit of a blunt instrument in communities like this and there are good alternatives to keep the discussion going.
However, my read on this does align with @mosermw in that this does look like another SFTP implementation so I too feel like this needs more work if we want to support SCP which offers subtle differences that may be preferred in some cases over SFTP. Knowing Mike I knew to take his -1 with nothing but noble intent and his follow-on comments support that as well.
Jsch is likely a perfectly fine library for this but as mike points out there are different api calls that are likely to be used for SCP.
@eorgad Would be curious to hear your thoughts on some good next steps to take with this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #453: Nifi 539c
Posted by eorgad <gi...@git.apache.org>.
Github user eorgad closed the pull request at:
https://github.com/apache/nifi/pull/453
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220166172
If SCPTransfer.java continues to use Jsch then I was expecting an implementation similar to Jsch example code for scp.
http://www.jcraft.com/jsch/examples/ScpTo.java.html
http://www.jcraft.com/jsch/examples/ScpFrom.java.html
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #453: Nifi 539c
Posted by trixpan <gi...@git.apache.org>.
Github user trixpan commented on the issue:
https://github.com/apache/nifi/pull/453
@eorgad , this PR is nearly 1 year old do you still plan to address some of the feedback? Would you mind closing the PR for now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by eorgad <gi...@git.apache.org>.
Github user eorgad commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220145092
Yes they are using jcraft.jsch under the cover - what did you have in mind?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by eorgad <gi...@git.apache.org>.
Github user eorgad commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220467272
Taking a dipper look at this, the the ScpTo.java code uses the following:
...
// exec 'scp -t rfile' remotely
String command="scp " + (ptimestamp ? "-p" :"") +" -t "+rfile;
Channel channel=session.openChannel("exec");
((ChannelExec)channel).setCommand(command);
// get I/O streams for remote scp
OutputStream out=channel.getOutputStream();
InputStream in=channel.getInputStream();
...
So implementing this would probably be the equivalent of invoking an ExecuteProcess
and providing it with the parameters for scp command. So the same question @mosermw
raised where is the value in those processors if I would end up with the same implementation
as ExecuteProcess. In both cases using the native binaries for scp.
Also as I am new to contributing to Nifi looks like the verification checks failed, when I look
at the build it succeeded. Am I doing something wrong procedurally or did it time out before the
build completed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: Nifi 539c
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the pull request:
https://github.com/apache/nifi/pull/453#issuecomment-220137783
Maybe I'm missing something, but it seems that the GetSCP and PutSCP processors here are still using SFTP under the covers. If this is the case, then -1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---