Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opening files. Download new copy or reuse local? #458

Closed
Colin-Stone opened this issue Jan 17, 2020 · 32 comments
Closed

Opening files. Download new copy or reuse local? #458

Colin-Stone opened this issue Jan 17, 2020 · 32 comments
Labels
enhancement New feature or request priority-low Legit issue but cosmetic or nice-to-have USS

Comments

@Colin-Stone
Copy link
Member

One concern that has been raised to me regarding the editor is that when opening a file, Zowe Explorer always tries to use an existing local version of a dataset or USS file rather than download a fresh version of the file. Although this is fine for files that don't change, if you are looking at a file that frequently changes it may be preferable to download every time.

We do provide the download icon to override the "already exists check" but apart from very big files is it a massive overhead to download each and everytime the user clicks on the file name in the explorer?.

Options are

  1. Add a setting with the default being download always. (or not)
  2. Base the decision on file size. If relatively small always download.
@MikeBauerCA
Copy link
Contributor

My opinion is that clicking the name of the data-set or USS file in the tree would download a fresh version of the file given that it isn't currently being edited in a VSCode frame. When I start editing a file, I would like to start with the latest version to reduce possible merge conflicts.

However, we need to keep in mind the use case where someone is using the tree to navigate between files while editing. I would not want my local changes to be overridden in that case. Personally though, I do find it odd that the Explorer tries to use a local version even after the local version has been closed in the editor.

@jelaplan
Copy link

Could you open the local file and concurrently do a diff check on the remote file and if a diff is found, notify the user?

@stepanzharychevbroadcom
Copy link
Contributor

stepanzharychevbroadcom commented Jan 20, 2020

@Colin-Stone @jelaplan I analysed what we currently have for this problem, the situation is we already have implemented conflict solver which analyses ETags. The only case which should be covered looks like this:

  1. User has local copy of file downloaded and closed in explorer;
  2. User opens the file; <=== That's a place where potential auto-download should happen
  3. User edits the file;
  4. User tries to save and gets unnecessary merge conflict, because file was modified before he actually opened it;

So for (2) I think we need to pre-download the file before user actually sees it to override the saved content. But it should be done wisely taking into account the following rules:

  1. We should check that file actually was changed on mainframe side, it can be done via this method using If-None-Match header, so we won't download anything in the case if local ETag matches remote one;
  2. We should limit size of file which is suitable for such auto-update (maybe 10 Mb for start);
  3. If file wasn't updated for certain amount of time and there's a local copy, we should probably show special sign for user, so he knows that may be it's a good time to update (I will detail this suggestion in separate issue about explorer for USS);

Please, let me know about your opinion.

@Colin-Stone
Copy link
Member Author

Hi Stepan. I had a suggestion from a user today that if a they have closed a file that it should always warrant reloading it if opened again. The rationale being that if he wanted to look at it again in it's current state he would have left it open. It's sort of logical and creates less surprise I think. I would also apply that rule to +10MB rather than add too many caveats

@stepanzharychevbroadcom
Copy link
Contributor

Hey @Colin-Stone, that's exactly what I tried to describe. Point (1) is just for warranting us that download will be done only when file was really changed on the mainframe side.
Can you, please, clarify what's exactly the problem with 10Mb rule to your mind?

@Colin-Stone
Copy link
Member Author

Colin-Stone commented Jan 20, 2020

If you have downloaded a big +10MB file and subsequently closed it. I would expect that by reopening it to get a new copy downloaded rather than the one I closed earlier. I think we may be lucky with machines that are pretty quick so I appeciate this may not be the case for everyone. Saving a local copy was done for performance reasons but I have aways felt it confuses more users than the extra time downloading annoys users. What does everyone else reckon?.

@stepanzharychevbroadcom
Copy link
Contributor

@Colin-Stone I think we still can compare ETag, but we should definitely ask user if he wants to download such files. Not everyone has great internet connection and traffic might matter here.

@stepanzharychevbroadcom
Copy link
Contributor

Also, as I promised I tried to formulate other connected enhancements which can be done before this one: #461

@Colin-Stone
Copy link
Member Author

Considering again and unless we get further ideas I am happy if you go ahead with your original suggestions.

@stepanzharychevbroadcom
Copy link
Contributor

@Colin-Stone I will assign it to myself then.

@zFernand0
Copy link
Member

I think we still can compare ETag, but we should definitely ask user if he wants to download such files. Not everyone has great internet connection and traffic might matter here.

Checking to see if the ETag changed sounds like a good idea.
It seems that when retrieving the contents, we can provide the If-None-Match header and handle the HTTP 304 Not Modified response to use the local file?
@Alexandru-Dumitru, any thoughts?

@venkatzhub
Copy link

Was the classic approach of comparing timestamps to decide whether to download or not considered?

@Alexandru-Dumitru
Copy link

I think we still can compare ETag, but we should definitely ask user if he wants to download such files. Not everyone has great internet connection and traffic might matter here.

Checking to see if the ETag changed sounds like a good idea.
It seems that when retrieving the contents, we can provide the If-None-Match header and handle the HTTP 304 Not Modified response to use the local file?
@Alexandru-Dumitru, any thoughts?

Yes, we can use it. As I discussed with @stepanzharychevbroadcom, it would save us some unnecessary downloads.

Was the classic approach of comparing timestamps to decide whether to download or not considered?

I would see how we can benefit from this approach as well, as we would only use a list call requesting the necessary attributes. We would need to make a PoC to use mtime instead of etag to make sure it covers our use cases. Also list would be less resource hungry when we discuss larger files.

For example, currently the merge conflict is generated as follows:

PC REST Call MF
file1 <---etag(1)----(download) file1 (etag1)
file1(etag1) file1 (etag2) get's modified
file1(etag1) (upload) ------etag(1)----> file1 (etag2) Req. denied ! eta1 != eta2
file1(etag1) <----etag(2)---(download) file1 (etag2)
file1(etag1) < merge conflict> file1(etag2)*
file1(etag2) (upload) ----(etag2) ---> file1(etag2) Req. accepted

*this is generated by rewriting the content of the file on disk, without modifying the content of the file displayed by VS Code, then calling Save() function of VS Code. At this point, VS Code detects the content on disk is different, and generates the merge conflict panels. Upon resolving, we try to upload again with the new etag, thus going through the same process all over.

@zFernand0
Copy link
Member

Was the classic approach of comparing timestamps to decide whether to download or not considered?

I believe that ETag will serve the same purpose as a timestamp comparison : )

@venkatzhub
Copy link

@Alexandru-Dumitru - this problem has been solved numerous times using the timestamps (translates to - approach is proved in the field and a variety of real customers shops), and to your point that approach has not been resource intensive, however I don't know how it compares to ETag. So a quick POC would be worthwhile.

@dkelosky
Copy link
Contributor

@Alexandru-Dumitru is your PoC proposal only for the purpose of getting a more up-to-date local copy?

It seems the Save() scenario would still require the use of ETag. Do you agree?

@Alexandru-Dumitru
Copy link

@Alexandru-Dumitru is your PoC proposal only for the purpose of getting a more up-to-date local copy?

It seems the Save() scenario would still require the use of ETag. Do you agree?

@dkelosky I was thinking to look into the possibility to replace etag with mtime, to see if it's worth while, or some combination of the two.

@stepanzharychevbroadcom
Copy link
Contributor

@Alexandru-Dumitru I don't think that mtime is reliable here, I still can think about the scenario where do unnecessary download. The case is:

  1. User has file (let's call it version#1) which is similar by mtime with mainframe;
  2. Other user modifies the same file on the mainframe side once (version#2), but after he changes it back to version#1;
  3. User opens this same file which is identical to his local copy and does unnecessary download, because mtime is different;

So I guess the best one would be still to extend Zowe CLI with ETag check and download only if local and remote don't match.

@venkatzhub
Copy link

venkatzhub commented Jan 27, 2020

@stepan Zharychev - while that is a legit scenario - how often does that happen in the real world?

@dkelosky
Copy link
Contributor

@venkatzhub - Agreed it may not happen often and worst case there is an unnecessary download.

For upload (Save()), it seems there could be a window for an unintentional overlay if ETag is not used.

In the client code with an mtime only implementation and during a Save() operation, we could compare times and determine to upload (which is a z/OSMF REST to write).

Behind the scenes this starts a TSO address space. Depending on workload and policy, this could take some time, during which, the file could be altered via ISPF or some other means. A completed upload after this could overwrite content.

Perhaps this would not occur often, but the consequences might be a bit more severe. What do you think?

@Alexandru-Dumitru
Copy link

Going through https://tools.ietf.org/html/rfc7232 (HTTP conditional requests) and z/OSMF documentation and it seems that z/OSMF implements only strong validation with entity tags. There is no header to pass Last-Modified Dates to handle , meaning that all the validation logic should be taken care by us (the client). I am not objecting this, just wanted to highlight it.

@venkatzhub
Copy link

@dkelosky - the consequence of overwriting are indeed a bit more severe, however the chances are still low. But using ETag on Save is worthwhile to prevent any possible loss of data cases.

@dkelosky
Copy link
Contributor

Hi @Alexandru-Dumitru - sorry but I don't fully understand the link. z/OSMF implements strong validation but validation should be taken care of by the client?

@Alexandru-Dumitru
Copy link

Hi @Alexandru-Dumitru - sorry but I don't fully understand the link. z/OSMF implements strong validation but validation should be taken care of by the client?

Sorry for the confusion created. I meant that z/OSMF only implements Etags to address conditional processing of HTTP requests. They have the option to supply and accept mtime as well (mentioned in the RFC) by returning it for the resources in case and also accept it back within some headers, but it is not there. As a result, we would have to implement the request, validation and conditional processing of mtime in the client (zowe/cli) and not to count on z/OSMF for this. With this being said, I would like to side with etag going forward.

@jellypuno
Copy link
Contributor

@stepanzharychevbroadcom is this fixed? if yes, can you please close the issue?

@stepanzharychevbroadcom
Copy link
Contributor

@jellypuno It's still pending. So let's keep it for now.

@jellypuno jellypuno added the USS label Mar 5, 2020
@travatine
Copy link

Hi,

+1 for adding a preference " always download" with default "on".

@zFernand0
Copy link
Member

Hi,

+1 for adding a preference " always download" with default "on".

Hey @travatine, (and everyone on the thread)
Feel free to vote here: #663

@Colin-Stone
Copy link
Member Author

Adding a note that with Jobs. If it is Active, it should always have everything downloaded again

@zdmullen
Copy link
Contributor

We want to implement the following approach, following the poll:

Add a Setting. Default: Always Download

@zdmullen zdmullen added this to the Developer milestone May 27, 2020
@zdmullen zdmullen modified the milestones: Developer, 1.8 Release Jun 25, 2020
@crawr crawr modified the milestones: 1.8 Release, 1.9 Release Jul 31, 2020
@zdmullen zdmullen modified the milestones: 1.9 Release, Developer Aug 6, 2020
@JillieBeanSim JillieBeanSim removed this from the Developer milestone Oct 6, 2022
@zFernand0 zFernand0 added enhancement New feature or request priority-low Legit issue but cosmetic or nice-to-have labels Jan 17, 2023
@github-actions
Copy link

Thank you for raising this issue.
The community has 90 days to upvote 👍 the issue.
If it receives 10 upvotes, we will move it to our backlog. If not, we will close it.

@JillieBeanSim
Copy link
Contributor

This enhancement has had no community activity for 12 months. The issue also has less than 10 up-votes by the community. No action on this enhancement is targeted for the next 2 calendar quarters. Therefore, this enhancement is being closed. If you feel that this enhancement should continue to be available for community up-votes, you may reopen this issue.

@JillieBeanSim JillieBeanSim closed this as not planned Won't fix, can't repro, duplicate, stale Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-low Legit issue but cosmetic or nice-to-have USS
Projects
None yet
Development

No branches or pull requests