wgtmac commented on code in PR #405:
URL: https://github.com/apache/iceberg-cpp/pull/405#discussion_r2605692046
##########
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_);
Review Comment:
Shouldn't we return an error?
##########
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:
It might be delayed until register is successful. Or we can add a new
lockless `LoadTableImpl` to wrap the internal.
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -21,8 +21,9 @@
#include <algorithm>
#include <iterator>
-#include <mutex>
+#include <shared_mutex>
Review Comment:
This seems redundant.
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -21,8 +21,9 @@
#include <algorithm>
#include <iterator>
-#include <mutex>
+#include <shared_mutex>
+#include "iceberg/exception.h"
Review Comment:
Please not add this since we cannot throw in most of the codebase.
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -387,48 +388,50 @@ Result<std::unique_ptr<Table>>
InMemoryCatalog::CreateTable(
const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
+ std::unique_lock lock(mutex_);
return NotImplemented("create table");
}
Result<std::unique_ptr<Table>> InMemoryCatalog::UpdateTable(
const TableIdentifier& identifier,
const std::vector<std::unique_ptr<TableRequirement>>& requirements,
const std::vector<std::unique_ptr<TableUpdate>>& updates) {
+ std::unique_lock lock(mutex_);
return NotImplemented("update table");
}
Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable(
const TableIdentifier& identifier, const Schema& schema, const
PartitionSpec& spec,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
+ std::unique_lock lock(mutex_);
return NotImplemented("stage create table");
}
Result<bool> InMemoryCatalog::TableExists(const TableIdentifier& identifier)
const {
- std::lock_guard guard(mutex_);
+ std::shared_lock lock(mutex_);
return root_namespace_->TableExists(identifier);
}
Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool
purge) {
- std::lock_guard guard(mutex_);
+ std::unique_lock lock(mutex_);
// TODO(Guotao): Delete all metadata files if purge is true.
return root_namespace_->UnregisterTable(identifier);
}
Status InMemoryCatalog::RenameTable(const TableIdentifier& from,
const TableIdentifier& to) {
+ std::unique_lock lock(mutex_);
return NotImplemented("rename table");
}
Result<std::unique_ptr<Table>> InMemoryCatalog::LoadTable(
const TableIdentifier& identifier) {
- if (!file_io_) [[unlikely]] {
Review Comment:
Why this gets removed?
##########
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:
+1 on `std::lock_guard` if we care the performance 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]
