Fix logic bug in external file clean up #956

zitrosolrac · 2021-09-09T19:53:48Z

…rnal.py

dimitri-yatsenko · 2021-09-09T22:06:35Z

datajoint/external.py

@@ -338,7 +338,7 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
            error_list = []
            for uuid, external_path in items:
                try:
-                    count = (self & {'hash': uuid}).delete_quick(get_count=True)  # optimize
+                    count = len(self & {'hash': uuid})  # optimize
                except Exception:
                    pass   # if delete failed, do not remove the external file


The comment is stale. No delete has been attempted.

dimitri-yatsenko · 2021-09-09T22:07:52Z

datajoint/external.py

@@ -338,7 +338,7 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
            error_list = []
            for uuid, external_path in items:
                try:
-                    count = (self & {'hash': uuid}).delete_quick(get_count=True)  # optimize
+                    count = len(self & {'hash': uuid})  # optimize
                except Exception:
                    pass   # if delete failed, do not remove the external file


What exception is expected from the len query? Why is this helpful?

dimitri-yatsenko · 2021-09-09T22:08:24Z

datajoint/external.py

@@ -338,7 +338,7 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
            error_list = []
            for uuid, external_path in items:
                try:
-                    count = (self & {'hash': uuid}).delete_quick(get_count=True)  # optimize
+                    count = len(self & {'hash': uuid})  # optimize


Why do we need to get the count? Why do we need an extra query rather than deleting directly?

dimitri-yatsenko · 2021-09-09T22:09:22Z

datajoint/external.py

@@ -347,6 +347,8 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
                        self._remove_external_file(external_path)


Why is it better to remove the external file before deleting from the external table. What if the delete from the external table fails?

guzman-raphael

@zitrosolrac Thanks for the PR! 🤝

Need a few more refinements to the delete algorithm, test, and a bit more clean up.
Also, make sure to remove the binary files that were accidentally committed.

guzman-raphael · 2021-09-09T21:34:42Z

tests/__init__.py

@@ -20,7 +20,7 @@
 __author__ = 'Edgar Walker, Fabian Sinz, Dimitri Yatsenko, Raphael Guzman'

 # turn on verbose logging
-logging.basicConfig(level=logging.DEBUG)
+#logging.basicConfig(level=logging.DEBUG)


Uncomment this. Let's not commit debug changes.

Suggested change

#logging.basicConfig(level=logging.DEBUG)

logging.basicConfig(level=logging.DEBUG)

guzman-raphael · 2021-09-09T21:35:57Z

tests/test_external.py

-from .schema_external import SimpleRemote
+from .schema_external import stores_config, SimpleRemote, Simple, schema
+
+import json


Seems like this is no longer necessary.

Suggested change

import json

guzman-raphael · 2021-09-09T21:36:18Z

tests/test_external.py

+
+import json
+import os 
+from os import stat


No longer necessary

Suggested change

from os import stat

guzman-raphael · 2021-09-09T21:36:37Z

tests/test_external.py

+import pwd
+from pwd import getpwuid


Suggested change

import pwd

from pwd import getpwuid

guzman-raphael · 2021-09-09T21:37:57Z

tests/test_external.py

@@ -21,32 +26,14 @@ def tearDown(self):
    dj.config['stores']['local']['location'] = current_location_local


-def test_external_put():


Why was this deleted?

guzman-raphael · 2021-09-09T21:39:30Z

tests/test_external.py

+def test_remove_fail():
+    #https://github.com/datajoint/datajoint-python/issues/953
+
+    #print(json.dumps(dj.config['stores'], indent=4))


These unnecessary comments should be cleaned up if they don't contain notes.

guzman-raphael · 2021-09-09T21:40:30Z

tests/test_external.py

+
+    #print('location')
+    # print('\n IN TEST: BEFORE DELETE: list of dir stores, local, location')
+    print('stores location -----------\n')


These prints do not look like they are necessary for the test.

guzman-raphael · 2021-09-09T21:41:48Z

tests/test_external.py

+    path1 = dj.config['stores']['local']['location'] + '/djtest_extern/4/c/'
+
+    argDir = dj.config['stores']['local']['location'] + '/djtest_extern/4/c/'


Could be simplified to one variable.

guzman-raphael · 2021-09-09T22:31:11Z

datajoint/external.py

@@ -338,7 +338,7 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
            error_list = []
            for uuid, external_path in items:
                try:


Looks like lines 340-345 are unnecessary here.

datajoint/external.py

guzman-raphael · 2021-09-09T23:52:54Z

@zitrosolrac oh forgot one thing. Can you please update the changelog and doc-parts release notes as well.

…delete method, and re write test_removal_fail

guzman-raphael

@zitrosolrac Nice work man! Just some minor things left. 👏

datajoint/external.py

tests/test_external.py

Co-authored-by: Raphael Guzman <[email protected]>

…c/datajoint-python into external_removal_fail

guzman-raphael

@zitrosolrac 🎉 WooHoo! 🎉 Nice job on a tricky one 👍

dimitri-yatsenko · 2021-09-12T16:46:50Z

datajoint/external.py

+                            self._remove_external_file(external_path)
+                        except Exception as error:
+                            # adding row back into table after failed delete
+                            self.insert1(row[0])


set skip_duplicates=True to address potential race conditions.

I think it's okay not to re-insert if the delete fails. The procedure will return the undeleted files to the user for special handling. Without implementing an actual transaction management process, we can allow some orphaned files in the external storage and provide an additional cleanup utility.

@dimitri-yatsenko That is a good suggestion for the skipping duplicates. Regarding the cleanup, I feel this is actually necessary. There is basically 2 primary concerns:

The user has only one chance to catch the errors. Meaning that running the external delete/clean up multiple times will yield no change though it has only been removed from the external tracking table. This gives a false sense that the system has been 'cleaned-up' but it has not been removed from the actual store (this is the main complaint from the user in issue ExternalTable.delete should not remove row on error #953). We can provide an additional utility to resolve this but that would be expensive to run as it would have to 'crawl' the entire store to find objects that don't exist within the external tracking table. By simply inserting it back when there is an exception, we are allowing the error to be more visible but most importantly reproducible.

schema.external[store].delete() is not currently documented that it returns a list of the errors. Users are most likely unaware of this feature and therefore aren't using it properly. @zitrosolrac Could you open an issue on this in our datajoint-docs and reference it in ExternalTable.delete should not remove row on error #953? (Filing for now since our new team members haven't been oriented to the docs setup yet).

Suggested change

self.insert1(row[0])

self.insert1(row[0], skip_duplicates=True)

@guzman-raphael Got it! I'll open an issue on this in the datajoint-docs and add the necessary reference.

guzman-raphael · 2021-09-13T22:12:05Z

datajoint/external.py

+                            self._remove_external_file(external_path)
+                        except Exception as error:
+                            # adding row back into table after failed delete
+                            self.insert1(row[0])


Suggested change

self.insert1(row[0])

self.insert1(row[0], skip_duplicates=True)

Co-authored-by: Raphael Guzman <[email protected]>

Carlos Ortiz and others added 7 commits August 31, 2021 18:22

add test placeholder.

4a64615

Add test file to see file location and permissions.

f41b8d1

Added new print statements and added new print statements within exte…

ad32b6e

…rnal.py

adding more asserts to test file

7b3600a

adding new func to delete in external.py

e244337

added code to test

6452838

Uncomment to re-enable test

e0bd234

zitrosolrac force-pushed the external_removal_fail branch from 49206ef to e0bd234 Compare September 9, 2021 20:04

zitrosolrac added 3 commits September 9, 2021 15:10

Update lnx-docker and local-docker files with correct version of image

8558adb

Fix issues with style tests in external.py

56cf8b7

Remove print statements

84e4aa4

zitrosolrac marked this pull request as ready for review September 9, 2021 21:11

zitrosolrac requested a review from guzman-raphael September 9, 2021 21:11

dimitri-yatsenko requested changes Sep 9, 2021

View reviewed changes

guzman-raphael requested changes Sep 9, 2021

View reviewed changes

Ignore FileNotFound Exception when deleting, optimize external table …

5aa9b1e

…delete method, and re write test_removal_fail

guzman-raphael requested changes Sep 10, 2021

View reviewed changes

datajoint/external.py Show resolved Hide resolved

tests/test_external.py Outdated Show resolved Hide resolved

zitrosolrac and others added 6 commits September 10, 2021 16:01

Update CHANGELOG and DOCS-PART

957c563

Merging changes from master

b15fe43

Update tests/test_external.py

cdae1d5

Co-authored-by: Raphael Guzman <[email protected]>

Fixed parenthesis issue and uncommented logging console.

554fc71

Merge branch 'external_removal_fail' of https://github.com/zitrosolra…

f3436f0

…c/datajoint-python into external_removal_fail

Fix return statement placement

d4b972f

guzman-raphael approved these changes Sep 10, 2021

View reviewed changes

dimitri-yatsenko reviewed Sep 12, 2021

View reviewed changes

guzman-raphael requested changes Sep 13, 2021

View reviewed changes

Update datajoint/external.py

f8e8196

Co-authored-by: Raphael Guzman <[email protected]>

guzman-raphael approved these changes Sep 13, 2021

View reviewed changes

dimitri-yatsenko approved these changes Sep 14, 2021

View reviewed changes

Merge branch 'master' into external_removal_fail

707953b

dimitri-yatsenko merged commit 77fefd4 into datajoint:master Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix logic bug in external file clean up #956

Fix logic bug in external file clean up #956

zitrosolrac commented Sep 9, 2021

dimitri-yatsenko Sep 9, 2021

dimitri-yatsenko Sep 9, 2021

dimitri-yatsenko Sep 9, 2021

dimitri-yatsenko Sep 9, 2021

guzman-raphael left a comment

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael Sep 9, 2021

guzman-raphael commented Sep 9, 2021

guzman-raphael left a comment

guzman-raphael left a comment

dimitri-yatsenko Sep 12, 2021

dimitri-yatsenko Sep 12, 2021

guzman-raphael Sep 13, 2021 •

edited

Loading

guzman-raphael Sep 13, 2021

zitrosolrac Sep 14, 2021

guzman-raphael Sep 13, 2021

		@@ -347,6 +347,8 @@ def delete(self, *, delete_external_files=None, limit=None, display_progress=Tru
		self._remove_external_file(external_path)

	#logging.basicConfig(level=logging.DEBUG)
	logging.basicConfig(level=logging.DEBUG)

		@@ -21,32 +26,14 @@ def tearDown(self):
		dj.config['stores']['local']['location'] = current_location_local


		def test_external_put():

		path1 = dj.config['stores']['local']['location'] + '/djtest_extern/4/c/'

		argDir = dj.config['stores']['local']['location'] + '/djtest_extern/4/c/'

	self.insert1(row[0])
	self.insert1(row[0], skip_duplicates=True)

Fix logic bug in external file clean up #956

Fix logic bug in external file clean up #956

Conversation

zitrosolrac commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guzman-raphael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guzman-raphael commented Sep 9, 2021

guzman-raphael left a comment

Choose a reason for hiding this comment

guzman-raphael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guzman-raphael Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guzman-raphael Sep 13, 2021 •

edited

Loading