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

OverwriteNewerFile error on low-latency cloud calls #49

Open
pjbull opened this issue Aug 28, 2020 · 2 comments
Open

OverwriteNewerFile error on low-latency cloud calls #49

pjbull opened this issue Aug 28, 2020 · 2 comments
Labels
bug Something isn't working

Comments

@pjbull
Copy link
Member

pjbull commented Aug 28, 2020

Code like this will fail intermittently, likely because a transaction hasn't finished or a place where the cloud is updated before the local file edit time is updated.

s3p = S3Path("s3://pjb-testing-bucket-for-platform/file.txt")

s3p.touch()
s3p.exists()
s3p.write_text("a")
s3p.unlink()

With a traceback that looks like:

---------------------------------------------------------------------------
OverwriteNewerCloud                       Traceback (most recent call last)
 in 
      3 s3p.touch()
      4 s3p.exists()
----> 5 s3p.write_text("a")
      6 s3p.unlink()

~/code/cloudpathlib/cloudpathlib/cloudpath.py in write_text(self, data, encoding, errors)
    376             raise TypeError("data must be str, not %s" % data.__class__.__name__)
    377         with self.open(mode="w", encoding=encoding, errors=errors) as f:
--> 378             return f.write(data)
    379 
    380     # ====================== DISPATCHED TO POSIXPATH FOR PURE PATHS ======================

~/code/cloudpathlib/cloudpathlib/cloudpath.py in _patched_close(*args, **kwargs)
    310             def _patched_close(*args, **kwargs):
    311                 original_close(*args, **kwargs)
--> 312                 self._upload_local_to_cloud(force_overwrite_to_cloud=force_overwrite_to_cloud)
    313 
    314             buffer.close = _patched_close

~/code/cloudpathlib/cloudpathlib/cloudpath.py in _upload_local_to_cloud(self, force_overwrite_to_cloud)
    605 
    606         # cloud is newer and we are not overwriting
--> 607         raise OverwriteNewerCloud(
    608             f"Local file ({self._local}) for cloud path ({self}) is newer in the cloud disk, but "
    609             f"is being requested to be uploaded to the cloud. Either (1) redownload changes from the cloud or "

OverwriteNewerCloud: Local file (/var/folders/8g/v8lwvfhj6_l6ct_zd_rs84mw0000gn/T/tmp5jl8mhwn/pjb-testing-bucket-for-platform/file.txt) for cloud path (s3://pjb-testing-bucket-for-platform/file.txt) is newer in the cloud disk, but is being requested to be uploaded to the cloud. Either (1) redownload changes from the cloud or (2) pass `force_overwrite_to_cloud=True` to overwrite.
@pjbull
Copy link
Member Author

pjbull commented Oct 2, 2020

Learned a ton from: https://apenwarr.ca/log/20181113

Maybe ceil can help us here since mtime is only 1s resolution on linux

@jayqi
Copy link
Member

jayqi commented Oct 3, 2020

I ran into this with Azure Blob Storage after implementing the live cloud integration tests in #85.

tests/test_cloudpath_file_io.py::test_file_read_writes[rig_is_azure_rig] FAILED

        p.write_text("lalala")
>       assert p.read_text() == "lalala"

E cloudpathlib.cloudpath.OverwriteNewerLocal: Local file (/var/folders/nq/vp3dgt812jgb0q09rh5l706c0000gn/T/tmpgf7pcsg7/dd-test-container/test_cloudpath_file_io-test_file_read_writes/dir_0/file0_0.txt) for cloud path (az://dd-test-container/test_cloudpath_file_io-test_file_read_writes/dir_0/file0_0.txt) is newer on disk, but is being requested for download from cloud. Either (1) push your changes to the cloud, (2) remove the local file, or (3) pass force_overwrite_from_cloud=True to overwrite.

(Pdb) self._local.stat().st_mtime
1601690822.758994
(Pdb) self.stat().st_mtime
1601690822.0

It appears that Azure Blob Storage also only has second-resolution times.

One possible thing we can do is, every time there is an a > b check, we do ceil(a) > floor(b). That should handle inconsistent resolution between source and destination mtimes. It means that we potentially will do something wrong if someone is doing a bunch of writes to a file within a second though.

I also read the linked blog post. One problem they point out that we will not be able to solve this way is if the system clocks between the source and destination are not synced. I think this is a real concern because we're always comparing times across two different systems. The solution in the blog post is to maintain a separate database of mtimes. We may want to think about doing this as it's the only guaranteed way to deal with this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants