[ledger] Avoid calling IsDirectoryAt twice, when getting a LevelDb.
LedgerStorageImpl::GetPageStorage would check if the directory at the
given path existed, in order to return NOT_FOUND if necessary. It would
then call DbFactory::GetOrCreateDb, which checked again if the directory
exists.
In this CL DbFactory::GetOrCreateDb is updated to support only getting
an existing Db, and fail with NOT_FOUND status if it doesn't exist.
This helps remove the extra call to IsDirectoryAt in LedgerStorageImpl.
Test: LevelDbFactoryTest.*
Change-Id: I6b97615179ceae2347e9403b17177f44ee4e1112
diff --git a/bin/ledger/app/page_eviction_manager_impl.cc b/bin/ledger/app/page_eviction_manager_impl.cc
index 7cfb500..aa13c2a 100644
--- a/bin/ledger/app/page_eviction_manager_impl.cc
+++ b/bin/ledger/app/page_eviction_manager_impl.cc
@@ -118,38 +118,39 @@
// Initializing the DB and marking pages as closed are slow operations and we
// shouldn't wait for them to finish, before returning from initialization:
// Start these operations and finalize the initialization completer when done.
- coroutine_manager_.StartCoroutine(
- [this](coroutine::CoroutineHandler* handler) {
- ExpiringToken token = NewExpiringToken();
- if (!files::CreateDirectoryAt(db_path_.root_fd(), db_path_.path())) {
- initialization_completer_.Complete(Status::IO_ERROR);
- return;
- }
- storage::Status storage_status;
- std::unique_ptr<storage::Db> db_instance;
- if (coroutine::SyncCall(
- handler,
- [this](fit::function<void(storage::Status,
- std::unique_ptr<storage::Db>)>
- callback) {
- db_factory_->GetOrCreateDb(std::move(db_path_),
- std::move(callback));
- },
- &storage_status,
- &db_instance) == coroutine::ContinuationStatus::INTERRUPTED) {
- initialization_completer_.Complete(Status::INTERNAL_ERROR);
- return;
- }
- if (storage_status != storage::Status::OK) {
- initialization_completer_.Complete(
- PageUtils::ConvertStatus(storage_status));
- return;
- }
- db_ = std::make_unique<PageUsageDb>(environment_->clock(),
- std::move(db_instance));
- Status status = db_->MarkAllPagesClosed(handler);
- initialization_completer_.Complete(status);
- });
+ coroutine_manager_.StartCoroutine([this](
+ coroutine::CoroutineHandler* handler) {
+ ExpiringToken token = NewExpiringToken();
+ if (!files::CreateDirectoryAt(db_path_.root_fd(), db_path_.path())) {
+ initialization_completer_.Complete(Status::IO_ERROR);
+ return;
+ }
+ storage::Status storage_status;
+ std::unique_ptr<storage::Db> db_instance;
+ if (coroutine::SyncCall(
+ handler,
+ [this](fit::function<void(storage::Status,
+ std::unique_ptr<storage::Db>)>
+ callback) {
+ db_factory_->GetOrCreateDb(
+ std::move(db_path_), storage::DbFactory::OnDbNotFound::CREATE,
+ std::move(callback));
+ },
+ &storage_status,
+ &db_instance) == coroutine::ContinuationStatus::INTERRUPTED) {
+ initialization_completer_.Complete(Status::INTERNAL_ERROR);
+ return;
+ }
+ if (storage_status != storage::Status::OK) {
+ initialization_completer_.Complete(
+ PageUtils::ConvertStatus(storage_status));
+ return;
+ }
+ db_ = std::make_unique<PageUsageDb>(environment_->clock(),
+ std::move(db_instance));
+ Status status = db_->MarkAllPagesClosed(handler);
+ initialization_completer_.Complete(status);
+ });
return Status::OK;
}
diff --git a/bin/ledger/storage/fake/fake_db_factory.cc b/bin/ledger/storage/fake/fake_db_factory.cc
index e84bb32..e23c58d 100644
--- a/bin/ledger/storage/fake/fake_db_factory.cc
+++ b/bin/ledger/storage/fake/fake_db_factory.cc
@@ -12,14 +12,21 @@
namespace fake {
void FakeDbFactory::GetOrCreateDb(
- ledger::DetachedPath db_path,
+ ledger::DetachedPath db_path, DbFactory::OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<Db>)> callback) {
- // Create the path to fake the creation of the Db at the expected destination.
- if (!files::CreateDirectoryAt(db_path.root_fd(), db_path.path())) {
- FXL_LOG(ERROR) << "Failed to create the storage directory in "
- << db_path.path();
- callback(Status::INTERNAL_IO_ERROR, nullptr);
- return;
+ if (!files::IsDirectoryAt(db_path.root_fd(), db_path.path())) {
+ if (on_db_not_found == DbFactory::OnDbNotFound::RETURN) {
+ callback(Status::NOT_FOUND, nullptr);
+ return;
+ }
+ // Create the path to fake the creation of the Db at the expected
+ // destination.
+ if (!files::CreateDirectoryAt(db_path.root_fd(), db_path.path())) {
+ FXL_LOG(ERROR) << "Failed to create the storage directory in "
+ << db_path.path();
+ callback(Status::INTERNAL_IO_ERROR, nullptr);
+ return;
+ }
}
callback(Status::OK, std::make_unique<FakeDb>(dispatcher_));
}
diff --git a/bin/ledger/storage/fake/fake_db_factory.h b/bin/ledger/storage/fake/fake_db_factory.h
index f463079..7e56a89 100644
--- a/bin/ledger/storage/fake/fake_db_factory.h
+++ b/bin/ledger/storage/fake/fake_db_factory.h
@@ -17,7 +17,7 @@
: dispatcher_(dispatcher) {}
void GetOrCreateDb(
- ledger::DetachedPath db_path,
+ ledger::DetachedPath db_path, DbFactory::OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<Db>)> callback) override;
private:
diff --git a/bin/ledger/storage/impl/ledger_storage_impl.cc b/bin/ledger/storage/impl/ledger_storage_impl.cc
index ee31833..dca3246 100644
--- a/bin/ledger/storage/impl/ledger_storage_impl.cc
+++ b/bin/ledger/storage/impl/ledger_storage_impl.cc
@@ -75,7 +75,7 @@
auto timed_callback = TRACE_CALLBACK(std::move(callback), "ledger",
"ledger_storage_create_page_storage");
GetOrCreateDb(GetPathFor(page_id), std::move(page_id),
- std::move(timed_callback));
+ DbFactory::OnDbNotFound::CREATE, std::move(timed_callback));
}
void LedgerStorageImpl::GetPageStorage(
@@ -83,13 +83,8 @@
fit::function<void(Status, std::unique_ptr<PageStorage>)> callback) {
auto timed_callback = TRACE_CALLBACK(std::move(callback), "ledger",
"ledger_storage_get_page_storage");
- ledger::DetachedPath path = GetPathFor(page_id);
- if (!files::IsDirectoryAt(path.root_fd(), path.path())) {
- timed_callback(Status::NOT_FOUND, nullptr);
- return;
- }
-
- GetOrCreateDb(std::move(path), std::move(page_id), std::move(timed_callback));
+ GetOrCreateDb(GetPathFor(page_id), std::move(page_id),
+ DbFactory::OnDbNotFound::RETURN, std::move(timed_callback));
}
void LedgerStorageImpl::DeletePageStorage(
@@ -167,9 +162,10 @@
void LedgerStorageImpl::GetOrCreateDb(
ledger::DetachedPath path, PageId page_id,
+ DbFactory::OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<PageStorage>)> callback) {
db_factory_->GetOrCreateDb(
- std::move(path),
+ std::move(path), on_db_not_found,
callback::MakeScoped(
weak_factory_.GetWeakPtr(),
[this, page_id = std::move(page_id), callback = std::move(callback)](
diff --git a/bin/ledger/storage/impl/ledger_storage_impl.h b/bin/ledger/storage/impl/ledger_storage_impl.h
index 03cf890..b1f77aa 100644
--- a/bin/ledger/storage/impl/ledger_storage_impl.h
+++ b/bin/ledger/storage/impl/ledger_storage_impl.h
@@ -58,6 +58,7 @@
// given |page_id|.
void GetOrCreateDb(
ledger::DetachedPath path, PageId page_id,
+ DbFactory::OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<PageStorage>)> callback);
ledger::DetachedPath GetPathFor(PageIdView page_id);
diff --git a/bin/ledger/storage/impl/leveldb_factory.cc b/bin/ledger/storage/impl/leveldb_factory.cc
index 16920d5..4434077 100644
--- a/bin/ledger/storage/impl/leveldb_factory.cc
+++ b/bin/ledger/storage/impl/leveldb_factory.cc
@@ -103,7 +103,7 @@
}
void LevelDbFactory::GetOrCreateDb(
- ledger::DetachedPath db_path,
+ ledger::DetachedPath db_path, DbFactory::OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<Db>)> callback) {
if (files::IsDirectoryAt(db_path.root_fd(), db_path.path())) {
// If the path exists, there is a LevelDb instance already there. Open and
@@ -112,6 +112,10 @@
std::move(callback));
return;
}
+ if (on_db_not_found == DbFactory::OnDbNotFound::RETURN) {
+ callback(Status::NOT_FOUND, nullptr);
+ return;
+ }
// If creating the pre-cached db failed at some point it will likely fail
// again. Don't retry caching anymore.
if (cached_db_status_ == Status::OK) {
diff --git a/bin/ledger/storage/impl/leveldb_factory.h b/bin/ledger/storage/impl/leveldb_factory.h
index f6d4d85..43aae40 100644
--- a/bin/ledger/storage/impl/leveldb_factory.h
+++ b/bin/ledger/storage/impl/leveldb_factory.h
@@ -36,7 +36,7 @@
// DbFactory:
void GetOrCreateDb(
- ledger::DetachedPath db_path,
+ ledger::DetachedPath db_path, DbFactory::OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<Db>)> callback) override;
private:
diff --git a/bin/ledger/storage/impl/leveldb_factory_unittest.cc b/bin/ledger/storage/impl/leveldb_factory_unittest.cc
index ddb62c4..e93ec77 100644
--- a/bin/ledger/storage/impl/leveldb_factory_unittest.cc
+++ b/bin/ledger/storage/impl/leveldb_factory_unittest.cc
@@ -60,7 +60,7 @@
std::unique_ptr<Db> db;
bool called;
db_factory_.GetOrCreateDb(
- db_path_.SubPath("db"),
+ db_path_.SubPath("db"), DbFactory::OnDbNotFound::CREATE,
callback::Capture(callback::SetWhenCalled(&called), &status, &db));
RunLoopUntilIdle();
ASSERT_TRUE(called);
@@ -77,7 +77,7 @@
// Close the previous instance and open it again.
db.reset();
db_factory_.GetOrCreateDb(
- db_path_.SubPath("db"),
+ db_path_.SubPath("db"), DbFactory::OnDbNotFound::RETURN,
callback::Capture(callback::SetWhenCalled(&called), &status, &db));
RunLoopUntilIdle();
ASSERT_TRUE(called);
@@ -91,6 +91,20 @@
});
}
+TEST_F(LevelDbFactoryTest, GetDbOnNotFound) {
+ // Try to get a non existing Db and expect a |NOT_FOUND| status.
+ Status status;
+ std::unique_ptr<Db> db;
+ bool called;
+ db_factory_.GetOrCreateDb(
+ db_path_.SubPath("db"), DbFactory::OnDbNotFound::RETURN,
+ callback::Capture(callback::SetWhenCalled(&called), &status, &db));
+ RunLoopUntilIdle();
+ ASSERT_TRUE(called);
+ EXPECT_EQ(Status::NOT_FOUND, status);
+ EXPECT_EQ(nullptr, db);
+}
+
TEST_F(LevelDbFactoryTest, CreateMultipleDbs) {
int N = 5;
Status status;
@@ -104,7 +118,7 @@
EXPECT_FALSE(files::IsDirectoryAt(path.root_fd(), path.path()));
db_factory_.GetOrCreateDb(
- path,
+ path, DbFactory::OnDbNotFound::CREATE,
callback::Capture(callback::SetWhenCalled(&called), &status, &db));
RunLoopUntilIdle();
EXPECT_TRUE(called);
@@ -131,6 +145,7 @@
db_factory_.GetOrCreateDb(
db_path_.SubPath(fxl::NumberToString(i)),
+ DbFactory::OnDbNotFound::CREATE,
callback::Capture(callback::SetWhenCalled(&called[i]), &statuses[i],
&dbs[i]));
}
@@ -156,13 +171,15 @@
std::unique_ptr<Db> db2;
db_factory_.GetOrCreateDb(
- path1, [&](Status status1, std::unique_ptr<Db> db1) {
+ path1, DbFactory::OnDbNotFound::CREATE,
+ [&](Status status1, std::unique_ptr<Db> db1) {
called1 = true;
EXPECT_EQ(Status::OK, status1);
EXPECT_NE(nullptr, db1);
db_factory_.GetOrCreateDb(
- path2, callback::Capture(callback::SetWhenCalled(&called2),
- &status2, &db2));
+ path2, DbFactory::OnDbNotFound::CREATE,
+ callback::Capture(callback::SetWhenCalled(&called2), &status2,
+ &db2));
});
RunLoopUntilIdle();
EXPECT_TRUE(called1);
diff --git a/bin/ledger/storage/public/db_factory.h b/bin/ledger/storage/public/db_factory.h
index 6c027b3..423b4de 100644
--- a/bin/ledger/storage/public/db_factory.h
+++ b/bin/ledger/storage/public/db_factory.h
@@ -15,13 +15,23 @@
// A factory for Db instances.
class DbFactory {
public:
+ // Defines the action to be taken if |GetOrCreate| is called for a path that
+ // doesn't already contain a Db.
+ enum class OnDbNotFound {
+ // |GetOrCreateDb| should return with a |NOT_FOUND| status.
+ RETURN,
+ // |GetOrCreateDb| should create a new Db instance.
+ CREATE
+ };
+
DbFactory() {}
virtual ~DbFactory() {}
- // Opens and returns an initialized instance of Db in the given |db_path|. If
- // the Db doesn't already exist, it creates it.
+ // Opens and returns an initialized instance of Db in the given |db_path|.
+ // Depending on the value of |on_db_not_found|, if the Db doesn't already
+ // exist, it either returns with NOT_FOUND status, or creates a new one.
virtual void GetOrCreateDb(
- ledger::DetachedPath db_path,
+ ledger::DetachedPath db_path, OnDbNotFound on_db_not_found,
fit::function<void(Status, std::unique_ptr<Db>)> callback) = 0;
};
