You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "mosermw (via GitHub)" <gi...@apache.org> on 2024/06/03 21:36:19 UTC
[PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
mosermw opened a new pull request, #8914:
URL: https://github.com/apache/nifi/pull/8914
# Summary
[NIFI-13159](https://issues.apache.org/jira/browse/NIFI-13159)
For PutFTP/PutSFTP conflict resolution strategy REPLACE, when DOT_RENAME or temp filename is used, delay the deletion of remote file until after temp file transfer completes.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ x Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 21
### UI Contributions
- [ ] NiFi is modernizing its UI. Any contributions that update the [current UI](https://github.com/apache/nifi/tree/main/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui) also need to be implemented in the [new UI](https://github.com/apache/nifi/tree/main/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi).
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #8914:
URL: https://github.com/apache/nifi/pull/8914#issuecomment-2218751355
Thanks for the update @mosermw, this looks like a good approach as the put method already considers the temporary file when deciding whether to run the rename command.
If the rename failed, it would throw an exception, which could occur if the delete failed for some reason. For troubleshooting, I believe it would be helpful to log the remote exception at the debug level, and include the remote filename that could not be deleted. That way, if the rename operation fails, there is the opportunity to enable additional logging that could explain more about what failed. Otherwise, I think this approach looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
Posted by "mosermw (via GitHub)" <gi...@apache.org>.
mosermw commented on PR #8914:
URL: https://github.com/apache/nifi/pull/8914#issuecomment-2220979223
Added debug logging if the delete command throws an exception. This is a good suggestion also because some static analysis tools do not like empty catch blocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
Posted by "mosermw (via GitHub)" <gi...@apache.org>.
mosermw commented on PR #8914:
URL: https://github.com/apache/nifi/pull/8914#issuecomment-2215302556
Thanks for looking at this PR, @exceptionfactory
> @mosermw can you provide some additional context for the reasons behind this proposed change in behavior?
As a use case, let's say I have a server running software that has an anxiety attack if a certain file doesn't exist, and it checks for that file every 5 seconds. I have a requirement to update that file periodically. I use the power of NiFi to generate the file contents and then PutSFTP the file into place. Currently, my server software sometimes panics because it checks for the file while I am updating it. This is because PutSFTP deletes the file first, transfers the file as ".filename" then renames to "filename". As my file gets larger, it can take more than 5 seconds to transfer.
After the change in this PR, the file will only not exist in the short period of time between an SFTP delete then rename.
I asked the question on Slack a while back and got positive feedback that this change would be useful. I should have put the link into the Jira ticket. https://apachenifi.slack.com/archives/C0L9S92JY/p1713799005100369
> Reviewing the code, this change introduces an additional `mlist()` command for FTP and `stat()` command for SFTP, for each file transferred through the corresponding Processors. Those commands require both file access and network communication, which could have an impact on high volume flows. Is there a particular reason for adding those calls prior to calling `delete()`? It seems like that should not be necessary.
You're absolutely right and I can do better. It doesn't hurt to call delete whether the destination file exists or not. I will modify the PR to remove those additional commands and test again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
Posted by "mosermw (via GitHub)" <gi...@apache.org>.
mosermw commented on PR #8914:
URL: https://github.com/apache/nifi/pull/8914#issuecomment-2218736871
> It is worth noting, however, that having some other independent process checking for the existence of a file is still bound to result in a race condition. This change may reduce the chances, but it does not sound like a complete solution.
Oh I totally agree, but sometimes we must do the best with the hand we are dealt. I made the changes and the PR did get much simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #8914:
URL: https://github.com/apache/nifi/pull/8914#issuecomment-2215330557
> Thanks for looking at this PR, @exceptionfactory
>
> > @mosermw can you provide some additional context for the reasons behind this proposed change in behavior?
>
> As a use case, let's say I have a server running software that has an anxiety attack if a certain file doesn't exist, and it checks for that file every 5 seconds. I have a requirement to update that file periodically. I use the power of NiFi to generate the file contents and then PutSFTP the file into place. Currently, my server software sometimes panics because it checks for the file while I am updating it. This is because PutSFTP deletes the file first, transfers the file as ".filename" then renames to "filename". As my file gets larger, it can take more than 5 seconds to transfer.
>
> After the change in this PR, the file will only not exist in the short period of time between an SFTP delete then rename.
>
> I asked the question on Slack a while back and got positive feedback that this change would be useful. I should have put the link into the Jira ticket. https://apachenifi.slack.com/archives/C0L9S92JY/p1713799005100369
>
> > Reviewing the code, this change introduces an additional `mlist()` command for FTP and `stat()` command for SFTP, for each file transferred through the corresponding Processors. Those commands require both file access and network communication, which could have an impact on high volume flows. Is there a particular reason for adding those calls prior to calling `delete()`? It seems like that should not be necessary.
>
> You're absolutely right and I can do better. It doesn't hurt to call delete whether the destination file exists or not. I will modify the PR to remove those additional commands and test again.
Thanks for the reply and additional background @mosermw, that is helpful.
At the core, I agree the amount of time between uploading and renaming should be minimal. If you are able to rework the approach and avoid introducing the additional status calls, it seems like a viable improvement.
It is worth noting, however, that having some other independent process checking for the existence of a file is still bound to result in a race condition. This change may reduce the chances, but it does not sound like a complete solution.
So with that said, I will take another look when you have posted some updates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #8914: NIFI-13159 PutFTP/PutSFTP change when delete happens on REPLACE
URL: https://github.com/apache/nifi/pull/8914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org