WZhuo commented on code in PR #405:
URL: https://github.com/apache/iceberg-cpp/pull/405#discussion_r2605286992
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -337,42 +337,42 @@ std::string_view InMemoryCatalog::name() const { return
catalog_name_; }
Status InMemoryCatalog::CreateNamespace(
const Namespace& ns, const std::unordered_map<std::string, std::string>&
properties) {
- std::lock_guard guard(mutex_);
+ std::unique_lock lock(mutex_);
Review Comment:
Using unique_lock and shared_lock with shared_mutex makes it clear that this
is a read-write lock, as seeing unique_lock immediately indicates that
exclusive (write) access is being acquired. And, in other mutex scenarios, use
lock_guard.
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -443,14 +446,20 @@ 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_);
+ ICEBERG_CHECK(file_io_, "file_io is not set for catalog {}", catalog_name_);
+ ICEBERG_ASSIGN_OR_RAISE(auto metadata,
+ TableMetadataUtil::Read(*file_io_,
metadata_file_location));
Review Comment:
Here's an issue: if a file is invalid, register returns failure, but the map
may already contain an entry. Subsequent calls to register will then return
"already exists".
Therefore, I think we should validate the file's legitimacy in advance,
ensuring that only valid table metadata can be successfully registered.
##########
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:
It is an atomicity issue between Register and Load. We can discuss it based
on the new code.
--
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]
