Locking for shares part 1 #11 #74

dagl · 2021-09-16T11:04:21Z

Locking for concurrently edited shared files. There was a need for a large refactoring because of duplicate code and not working tests.

diocas

I believe I see many changes not necessarily related to this PR (like cleanups, fixes here and there, comments). It's better to separate things into smaller and thematic PRs, which makes it easier to review and results in a cleaner history.

But, either way, I've done some request and questions. I think the biggest change is that we should make the locking transparent and integrate it inside the save/read API. Ah, and just lock all the time, not only on shared files. Also, let's try to optimize this and cache things + reduce the amount of calls we do to reva for any single/simple operation.

Thanks!

jupyter-config/jupyter_cs3_config.json

.gitignore

cs3api4lab/api/cs3_file_api.py

cs3api4lab/utils/file_utils.py

cs3api4lab/api/lock_manager.py

cs3api4lab/api/share_api_facade.py

cs3api4lab/logic/storage_logic.py

diocas

Sorry, it passed so much time since the initial review that I hope I didn't repeat myself. :)
Just some comments. Please rebase as well, as there are conflicts and we cannot merge.
Ans please resolve what is still open.
Thanks

cs3api4lab/api/cs3_file_api.py

cs3api4lab/api/lock_manager.py

cs3api4lab/logic/storage_logic.py

dagl · 2022-01-09T21:21:53Z

I'm sorry for the last time, I pushed incomplete commit. This time everything should be fine.

diocas · 2022-01-11T09:25:53Z

Can you please rebase? I'm seeing tons of commits that should not be here.

dagl · 2022-01-13T13:30:41Z

Can you please rebase? I'm seeing tons of commits that should not be here.

done

dagl · 2022-01-19T20:29:17Z

Can you please rebase? I'm seeing tons of commits that should not be here.

Actually wait, I didn't do the rebase correctly

lgtm-com · 2022-01-20T20:22:44Z

This pull request introduces 8 alerts when merging d6f4a1a into a8b828c - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

lgtm-com · 2022-01-20T20:31:56Z

This pull request introduces 8 alerts when merging 55eda18 into a8b828c - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

lgtm-com · 2022-01-20T20:50:12Z

This pull request introduces 8 alerts when merging e1276b2 into a8b828c - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

lgtm-com · 2022-01-21T15:57:37Z

This pull request introduces 8 alerts when merging 41616c5 into 6a188ba - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

diocas · 2022-01-24T13:39:31Z

@dagl the pipeline doesn't work because the extension is not ok. You have 2 folders (logic and utils) that are not treated as modules because they don't have the __init__.py file inside.
If you pip install the extension and do the command that fails in the pipeline, you see that it fails while trying to import.

dagl · 2022-01-24T19:06:04Z

@dagl the pipeline doesn't work because the extension is not ok. You have 2 folders (logic and utils) that are not treated as modules because they don't have the __init__.py file inside. If you pip install the extension and do the command that fails in the pipeline, you see that it fails while trying to import.

Ok thanks, it worked.

lgtm-com · 2022-01-24T19:06:28Z

This pull request introduces 8 alerts when merging 4f89f46 into 6ae505b - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

diocas

One extra round. Also check the lgtm review (i.e change the except: for except Exception:)

.gitignore

cs3api4lab/api/cs3_file_api.py

cs3api4lab/api/lock_manager.py

cs3api4lab/api/share_api_facade.py

setup.py

cs3api4lab/api/share_api_facade_ocmless.py

diocas · 2022-01-24T21:17:02Z

cs3api4lab/utils/file_utils.py

+        else:
+            content_len = len(content.decode('utf-8'))
+        # providing '0' as size leads to unexpected additional file creation


I pinged you in some PR because this was already fixed in REVA.

lgtm-com · 2022-01-25T15:11:08Z

This pull request introduces 8 alerts when merging 1893293 into 0a09092 - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

lgtm-com · 2022-01-25T18:26:14Z

This pull request introduces 8 alerts when merging 6883807 into 0a09092 - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'
3 for Unused import
1 for Testing equality to None
1 for 'import *' may pollute namespace

rebase sciencemesh:master

Co-authored-by: Diogo Castro <diogo.castro@cern.ch>

rebase

Co-authored-by: Diogo Castro <diogo.castro@cern.ch>

lgtm-com · 2022-01-31T22:08:26Z

This pull request introduces 1 alert and fixes 2 when merging d221696 into 258da93 - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

fixed alerts:

2 for Unused import

lgtm-com · 2022-01-31T22:25:56Z

This pull request fixes 2 alerts when merging eee2de4 into 258da93 - view on LGTM.com

fixed alerts:

2 for Unused import

diocas

Let's merge this after the questions I've asked (even the ones in previous reviews) are resolved.

diocas · 2022-02-01T14:39:31Z

cs3api4lab/utils/file_utils.py

+        else:
+            content_len = len(content.decode('utf-8'))
+        # providing '0' as size leads to unexpected additional file creation


You removed the code, right? (At least I'm not seeing it) So please remove the comment above as well.

cs3api4lab/api/lock_manager.py

lgtm-com · 2022-02-01T16:40:41Z

This pull request fixes 2 alerts when merging bb6cfed into 258da93 - view on LGTM.com

fixed alerts:

2 for Unused import

# Conflicts: # cs3api4lab/api/lock_manager.py

lgtm-com · 2022-02-01T19:09:08Z

This pull request fixes 2 alerts when merging 058ad2d into 258da93 - view on LGTM.com

fixed alerts:

2 for Unused import

diocas requested changes Oct 6, 2021

View reviewed changes

diocas reviewed Nov 24, 2021

View reviewed changes

cs3api4lab/api/cs3_file_api.py Outdated Show resolved Hide resolved

cs3api4lab/api/lock_manager.py Outdated Show resolved Hide resolved

cs3api4lab/api/lock_manager.py Outdated Show resolved Hide resolved

cs3api4lab/logic/storage_logic.py Show resolved Hide resolved

dagl closed this Jan 24, 2022

dagl reopened this Jan 24, 2022

diocas requested changes Jan 24, 2022

View reviewed changes

dagl and others added 12 commits January 31, 2022 23:00

Locking for shares part 1 #11

5685a8a

rebase sciencemesh:master

delete unnecessary files

863835e

correct method names in model_utils

3b05932

Update cs3api4lab/utils/file_utils.py

c27f5d1

Co-authored-by: Diogo Castro <diogo.castro@cern.ch>

*WIP* minor fixes

13aca87

changes for .conflict files

b81968d

add username to filename

1c4e9ae

remove /api/cs3/locks/file endpoint

8bf843c

add missing logic and tests fixes

62e20c2

Locking for shares part 1 #11

fbee607

rebase

correct method names in model_utils

f00babb

rebase

Update cs3api4lab/utils/file_utils.py

1c7a37b

Co-authored-by: Diogo Castro <diogo.castro@cern.ch>

dagl added 9 commits January 31, 2022 23:01

*WIP* minor fixes

7f68c93

changes for .conflict files

6cd897e

add username to filename

4f642dc

remove /api/cs3/locks/file endpoint

457d91a

fix broken rebase

dad6d78

change jupyter version do 3.0.6, update yarn.lock

6a42321

fix yarn.lock

56fcdab

add __init__.py

6229e9c

change .conflict filename

3930f77

code cleanup, minor fixes

eee2de4

diocas reviewed Feb 1, 2022

View reviewed changes

preserve extension in .conflict files

bb6cfed

dagl added 2 commits February 1, 2022 20:01

preserve extension in .conflict files

48863a0

Merge remote-tracking branch 'origin/locks_part_1' into locks_part_1

058ad2d

# Conflicts: # cs3api4lab/api/lock_manager.py

diocas approved these changes Feb 2, 2022

View reviewed changes

diocas merged commit deb18bc into sciencemesh:master Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Locking for shares part 1 #11 #74

Locking for shares part 1 #11 #74

dagl commented Sep 16, 2021

diocas left a comment

diocas left a comment •

edited

Loading

dagl commented Jan 9, 2022

diocas commented Jan 11, 2022

dagl commented Jan 13, 2022

dagl commented Jan 19, 2022

lgtm-com bot commented Jan 20, 2022

lgtm-com bot commented Jan 20, 2022

lgtm-com bot commented Jan 20, 2022

lgtm-com bot commented Jan 21, 2022

diocas commented Jan 24, 2022

dagl commented Jan 24, 2022

lgtm-com bot commented Jan 24, 2022

diocas left a comment

diocas Jan 24, 2022

lgtm-com bot commented Jan 25, 2022

lgtm-com bot commented Jan 25, 2022

lgtm-com bot commented Jan 31, 2022

lgtm-com bot commented Jan 31, 2022

diocas left a comment

diocas Feb 1, 2022

lgtm-com bot commented Feb 1, 2022

lgtm-com bot commented Feb 1, 2022

Locking for shares part 1 #11 #74

Locking for shares part 1 #11 #74

Conversation

dagl commented Sep 16, 2021

diocas left a comment

Choose a reason for hiding this comment

diocas left a comment • edited Loading

Choose a reason for hiding this comment

dagl commented Jan 9, 2022

diocas commented Jan 11, 2022

dagl commented Jan 13, 2022

dagl commented Jan 19, 2022

lgtm-com bot commented Jan 20, 2022

lgtm-com bot commented Jan 20, 2022

lgtm-com bot commented Jan 20, 2022

lgtm-com bot commented Jan 21, 2022

diocas commented Jan 24, 2022

dagl commented Jan 24, 2022

lgtm-com bot commented Jan 24, 2022

diocas left a comment

Choose a reason for hiding this comment

diocas Jan 24, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Jan 25, 2022

lgtm-com bot commented Jan 25, 2022

lgtm-com bot commented Jan 31, 2022

lgtm-com bot commented Jan 31, 2022

diocas left a comment

Choose a reason for hiding this comment

diocas Feb 1, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Feb 1, 2022

lgtm-com bot commented Feb 1, 2022

diocas left a comment •

edited

Loading