Skip to content
This repository has been archived by the owner on Aug 5, 2021. It is now read-only.

rsync for copy with progress reporting #202

Closed
wants to merge 6 commits into from

Conversation

MorganRodgers
Copy link
Contributor

@MorganRodgers MorganRodgers commented Aug 8, 2019

Attempting to replace copymitter with Rsync while still retaining the ability to update the UI with progress messages. I do not know why but progress messages are being consumed by the Nginx error log instead of being sent to the client. I am using the same method-I thought-to update the client as copymitter was using.

Two commits of interest:

  • ab69bf0 - I attempt to add progress messages
  • 635c8eb - I suspect that there might be something related to NodeJS Events and so wrap the Rsync command in an EventEmitter subclass

Unless we are able to get the messages out of the logs and to the client this branch offers no improvements over use_rsync_instead_of_copymitter.

After additional testing it appears to me that the defect is present in the production version as well. In production the first copy operation after page-load never shows progress. Subsequent copies actions do show progress messages.

Knowing that the defect exists in master I have opened a new issue: #204 and am declaring this branch a success.

ericfranz and others added 3 commits March 8, 2019 12:04
This commit expands on the previous work, and attempts (but fails) to implement progress updates.
@ericfranz
Copy link
Contributor

So, I know collectively a non-trivial amount of time was put into the rsync solution... but...

what about using https://www.npmjs.com/package/fs-extra instead?

Since it utilizes the base node.js fs library perhaps it is a safer option than requiring a new external dependency such as rsync. Since it would be best to make a patch release with this bug fix and release sooner than later...

I came across fs-extra from this discussion https://stackoverflow.com/questions/13786160/copy-folder-recursively-in-node-js. I do not know anything else beyond that, or benefits or drawbacks to the current solution, and have not closely read the code to consider how it is actually doing what it does.

@MorganRodgers
Copy link
Contributor Author

We can certainly try it. I'm curious why copymitter fails in the first place as all it is doing is reading the file stream and writing it back out at the new location: https://github.com/coderaiser/node-copymitter/blob/v1.8.13/lib/copymitter.js#L216-L220. fs-extra is using fs.copy: https://github.com/jprichardson/node-fs-extra/blob/master/lib/copy/copy.js#L96. I don't know if the Node standard lib item is more robust on Lustre.

@MorganRodgers MorganRodgers changed the title [WIP] - Use rsync instead of copymitter Morgan's attempt Use rsync instead of copymitter Morgan's attempt Aug 22, 2019
@MorganRodgers
Copy link
Contributor Author

MorganRodgers commented Aug 26, 2019

While working on the software requirements docs I realized that we already require rsync on the systems to use the Job Composer, and so using it here does not introduce a new dependency per se.

@MorganRodgers
Copy link
Contributor Author

MorganRodgers commented Aug 27, 2019

A strike against this implementation is the version of rsync available by default on Centos 6 (version 3.0.6 protocol version 30) does not have the --info argument that we are using to implement progress messages.

Morgan Rodgers added 2 commits August 28, 2019 11:03
This will result in no progress messages being sent to users who are on systems running older versions of Rsync like Centos 6.
@ericfranz ericfranz changed the title Use rsync instead of copymitter Morgan's attempt rsync for copy with progress reporting Aug 29, 2019
@ericfranz
Copy link
Contributor

We ended up disabling cloud commander's progress updating, which utilizes a web socket connection to facilitate, in #208.

The HTTP request initiated copy works, the WS initiated copy fails on Luster. By disabling progress updating we reduce the code paths to just the HTTP request and ensure that works on Lustre and other file systems. We will add progress tracking as a requirement for the files app replacement.

Since we disabled progress tracking, this fix is no longer applicable.

@ericfranz ericfranz closed this Sep 3, 2019
@ericfranz ericfranz deleted the use_rsync_instead_of_copymitter_morgan branch September 3, 2019 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants