Python HL SDK: use IfNoneMatch to support conditional write (aka exclusive creation) #9309
Conversation
skshetry
force-pushed
the
fix/exclusive-file-create-mode
branch
2 times, most recently
from
32a69f9 to
e69ea94
Compare
There was a problem hiding this comment.
Looks good, had a small comment related to a check in the code.
Also, if you can open a follow-up issues:
- Refactor: the exception handling should use
api_exception_handlerwith customer handler to address the specific if_none_match. This should align all the exceptions thrown based on the API to the SDK exception - Bug: it seems we have a resource leak while exception is thrown calling
closeand the upload/link throws an exception we continue to raise without closing the fd.
| @@ -538,6 +538,8 @@ def _upload_raw(self) -> lakefs_sdk.ObjectStats: | |||
| headers=headers, | |||
| body=self._fd) | |||
|
|
|||
| if self._mode.startswith("x") and resp.status == http.HTTPStatus.PRECONDITION_FAILED: | |||
There was a problem hiding this comment.
You can integrate this check into the _io_exception_handler to return the correct exception
There was a problem hiding this comment.
I want this exception to be raised only when If-None-Match is set.
So, I don't think it makes sense to move this to _io_exception_handler(). Adding a custom handler is an option, but that feels more complex.
| if 'x' in mode and obj.exists(): # Requires explicit create | ||
| raise ObjectExistsException | ||
|
|
There was a problem hiding this comment.
Discussed with @nopcoder and decided to reintroduce this check to fail early.
While this may impact performance, this check already existed, so this PR is not introducing any additional overhead.
…usive creation) Closes treeverse#7505. This replaces existing obj.exists() check with conditional writes with 'IfNoneMatch: *'. I have tried to maintain compat in terms of error raised, by raising `ObjectExistsError` on pre-condition failure.
skshetry
force-pushed
the
fix/exclusive-file-create-mode
branch
from
ca0e8ae to
b33478e
Compare
nopcoder
added
include-changelog
Change Description
Closes #7505.
This replaces existing
obj.exists()check with conditional writes using 'IfNoneMatch: *'. I have tried to maintain compat in terms of error raised, by raisingObjectExistsErroron pre-condition failure.Testing Details
How were the changes tested?
There is an existing
test_object_upload_existsthat covers this. I have extended this test by parametrizing it withpre_sign=True|False.Breaking Change?
Does this change break any existing functionality? (API, CLI, Clients)
No.
Additional info
Logs, outputs, screenshots of changes if applicable (CLI / GUI changes)
Contact Details
How can we get in touch with you if we need more info? (ex. [email protected])