HuaHuaY commented on code in PR #405:
URL: https://github.com/apache/iceberg-cpp/pull/405#discussion_r2605125616
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -443,12 +447,14 @@ Result<std::unique_ptr<Table>> InMemoryCatalog::LoadTable(
Result<std::shared_ptr<Table>> InMemoryCatalog::RegisterTable(
const TableIdentifier& identifier, const std::string&
metadata_file_location) {
- std::lock_guard guard(mutex_);
- if (!root_namespace_->NamespaceExists(identifier.ns)) {
- return NoSuchNamespace("table namespace does not exist.");
- }
- if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) {
- return UnknownError("The registry failed.");
+ {
+ std::unique_lock lock(mutex_);
+ if (!root_namespace_->NamespaceExists(identifier.ns)) {
+ return NoSuchNamespace("table namespace does not exist.");
+ }
+ if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) {
+ return UnknownError("The registry failed.");
+ }
}
return LoadTable(identifier);
Review Comment:
Is it possible that another thread drops the table between line 458 and 459?
If it's, what will happen? I think we may get an error `Table does not exist`
which looks strange in `RegisterTable`. I'm not sure if there are other
concurrency conflict scenarios here.
In the previous design, mutex_ is a recursive mutex. I think recursive mutex
is a bad design. But when we change it into a non-recursive mutex, we need to
be careful. If we think the current code is acceptable, I suggest to add some
comments here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
