Notimplemented handling #2108
Notimplemented handling #2108
Conversation
G-D-Petrov
requested review from
alexowens90,
willdealtry and
poodlewars
as code owners
| @@ -36,7 +36,7 @@ class NfsBackedStorage final : public Storage { | |||
| void do_write(Composite<KeySegmentPair>&& kvs) final; | |||
|
|
|||
| void do_write_if_none(KeySegmentPair&& kv [[maybe_unused]]) final { | |||
| storage::raise<ErrorCode::E_UNSUPPORTED_ATOMIC_OPERATION>("Atomic operations are only supported for s3 backend"); | |||
There was a problem hiding this comment.
The changes for NFS backed storages were made to make testing easier and to add the same general behavior as for S3 storages.
This can be reverted, if needed.
| @@ -74,6 +74,10 @@ namespace s3 { | |||
| raise<ErrorCode::E_ATOMIC_OPERATION_FAILED>(fmt::format("Atomic operation failed: {}", error_message_suffix)); | |||
| } | |||
|
|
|||
| if(err.GetExceptionName().find("NotImplemented") != std::string::npos) { | |||
There was a problem hiding this comment.
I assume there's no more robust way to figure this out? Not enough info in the type?
There was a problem hiding this comment.
The type of the error is UNKNOWN(100) is a but of a catch-all and might result in false positives.
I prefer to handle only this particular case.
| @@ -54,7 +54,7 @@ class NfsBackedStorage final : public Storage { | |||
| } | |||
|
|
|||
| bool do_supports_atomic_writes() const final { | |||
| return false; | |||
| return true; | |||
There was a problem hiding this comment.
This should still be false shouldn't it? Is this to make the testing easier like you mention above? I think it's too confusing having this true and then always blowing up when this backend does an atomic write.
Should be possible to test with some extensions to the mocked storages stuff used for the storages C++ tests?
There was a problem hiding this comment.
Please don't change those before I merge the storage refactor, it will make a huge amount of work for me.
I was considering getting rid of write_if_none in favour of having it as an option to the write method, as the API is getting quite crowded
There was a problem hiding this comment.
Added a way to configure this.
Having the ability to enable these writes for the nfs storage makes it easy to write an integration test for the ReliableLock in the enterprise repo
There was a problem hiding this comment.
Needs tests.
I think this looks sensible. I discussed with Ivo that we should perhaps add some logic to test a conditional write by actually writing an object to the backend, as the different S3 providers implement this very differently. But we'll probably want this logic anyway.
…ption in ReliableStorageLock
G-D-Petrov
force-pushed
the
notimplemented_handling
branch
from
af30193 to
b18b325
Compare
cpp/arcticdb/util/error_code.hpp
Outdated
| @@ -86,6 +86,7 @@ inline std::unordered_map<ErrorCategory, const char*> get_error_category_names() | |||
| ERROR_CODE(5020, E_UNEXPECTED_S3_ERROR) \ | |||
| ERROR_CODE(5021, E_S3_RETRYABLE) \ | |||
| ERROR_CODE(5022, E_ATOMIC_OPERATION_FAILED) \ | |||
| ERROR_CODE(5023, E_NOT_IMPLEMENTED) \ | |||
There was a problem hiding this comment.
Nit: This sounds a bit like something that we did not implement yet. It's more that the storage (PURE) does not support it.
What do you think about E_NOT_IMPLEMENTED_BY_STORAGE or E_NOT_SUPPORTED_BY_STORAGE?
There was a problem hiding this comment.
Let's implement Ivo's idea #2108 (comment) or we will never remember to come back and do it. Otherwise LGTM
Reference Issues/PRs
#203 in enterprise
What does this implement or fix?
Add a way to differentiate between a NotImplemented error and a regular StorageException.
This way we can properly handle S3 storages that haven't implemented the write_if_none operation.
Also updated the ReliableStorageLock to use the new behaviour.
Any other comments?
Checklist
Checklist for code changes...