From 4afaafdc770612185c4e11d3f09226ce168462a5 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Thu, 22 Mar 2018 15:45:05 -0700 Subject: Change write groups to use transactions (#912) * Start work on leveldb transactions * Style * Working API. Not plumbed in yet * Move files into correct place * Wrangling file locations and associations * Tests pass * Add some comments * style * Fix copyright * Rewrite iterator internals to handle deletion-while-iterating. Also add tests for same * Switch to strings instead of slices * Style * More style fixes * Start switching writegroup over * Swap out write group tracking for transaction usage * Style * Response to feedback before updating docs * Style * Add comment * Initialize version_ * Satisfy the linter * Start switching writegroup over * Swap out write group tracking for transaction usage * Style * Checkpoint before implementing BatchDescription * Style * Everything passing * Drop unused function * Style * Renaming * Style * Add log line * Style * Add debug log line for commits, drop unused BatchDescription --- Firestore/Source/Local/FSTLevelDB.mm | 26 +++---- Firestore/Source/Local/FSTLevelDBMigrations.h | 6 +- Firestore/Source/Local/FSTLevelDBMigrations.mm | 58 +++++++--------- Firestore/Source/Local/FSTLevelDBQueryCache.h | 8 +++ Firestore/Source/Local/FSTLevelDBQueryCache.mm | 25 +++++++ Firestore/Source/Local/FSTWriteGroup.h | 11 +-- Firestore/Source/Local/FSTWriteGroup.mm | 94 +++++++------------------- Firestore/Source/Local/FSTWriteGroupTracker.h | 5 ++ Firestore/Source/Local/FSTWriteGroupTracker.mm | 11 +++ 9 files changed, 122 insertions(+), 122 deletions(-) (limited to 'Firestore/Source/Local') diff --git a/Firestore/Source/Local/FSTLevelDB.mm b/Firestore/Source/Local/FSTLevelDB.mm index ac29304..a7ee99d 100644 --- a/Firestore/Source/Local/FSTLevelDB.mm +++ b/Firestore/Source/Local/FSTLevelDB.mm @@ -31,6 +31,7 @@ #include "Firestore/core/src/firebase/firestore/auth/user.h" #include "Firestore/core/src/firebase/firestore/core/database_info.h" +#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" @@ -43,6 +44,7 @@ NS_ASSUME_NONNULL_BEGIN static NSString *const kReservedPathComponent = @"firestore"; +using firebase::firestore::local::LevelDbTransaction; using leveldb::DB; using leveldb::Options; using leveldb::ReadOptions; @@ -58,7 +60,9 @@ using leveldb::WriteOptions; @end -@implementation FSTLevelDB +@implementation FSTLevelDB { + std::unique_ptr _transaction; +} /** * For now this is paranoid, but perhaps disable that in production builds. @@ -137,7 +141,9 @@ using leveldb::WriteOptions; return NO; } _ptr.reset(database); - [FSTLevelDBMigrations runMigrationsOnDB:_ptr]; + LevelDbTransaction transaction(_ptr.get()); + [FSTLevelDBMigrations runMigrationsWithTransaction:&transaction]; + transaction.Commit(); return YES; } @@ -212,20 +218,16 @@ using leveldb::WriteOptions; } - (FSTWriteGroup *)startGroupWithAction:(NSString *)action { - return [self.writeGroupTracker startGroupWithAction:action]; + FSTAssert(_transaction == nullptr, @"Starting a transaction while one is already outstanding"); + _transaction = std::make_unique(_ptr.get()); + return [self.writeGroupTracker startGroupWithAction:action transaction:_transaction.get()]; } - (void)commitGroup:(FSTWriteGroup *)group { + FSTAssert(_transaction != nullptr, @"Committing a transaction before one is started"); [self.writeGroupTracker endGroup:group]; - - NSString *description = [group description]; - FSTLog(@"Committing %@", description); - - Status status = [group writeToDB:_ptr]; - if (!status.ok()) { - FSTFail(@"%@ failed with status: %s, description: %@", group.action, status.ToString().c_str(), - description); - } + _transaction->Commit(); + _transaction.reset(); } - (void)shutdown { diff --git a/Firestore/Source/Local/FSTLevelDBMigrations.h b/Firestore/Source/Local/FSTLevelDBMigrations.h index 24fb5c8..1724edf 100644 --- a/Firestore/Source/Local/FSTLevelDBMigrations.h +++ b/Firestore/Source/Local/FSTLevelDBMigrations.h @@ -18,6 +18,7 @@ #include +#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" #include "leveldb/db.h" NS_ASSUME_NONNULL_BEGIN @@ -29,12 +30,13 @@ typedef int32_t FSTLevelDBSchemaVersion; /** * Returns the current version of the schema for the given database */ -+ (FSTLevelDBSchemaVersion)schemaVersionForDB:(std::shared_ptr)db; ++ (FSTLevelDBSchemaVersion)schemaVersionWithTransaction: + (firebase::firestore::local::LevelDbTransaction *)transaction; /** * Runs any migrations needed to bring the given database up to the current schema version */ -+ (void)runMigrationsOnDB:(std::shared_ptr)db; ++ (void)runMigrationsWithTransaction:(firebase::firestore::local::LevelDbTransaction *)transaction; @end diff --git a/Firestore/Source/Local/FSTLevelDBMigrations.mm b/Firestore/Source/Local/FSTLevelDBMigrations.mm index 7595c53..7f521d1 100644 --- a/Firestore/Source/Local/FSTLevelDBMigrations.mm +++ b/Firestore/Source/Local/FSTLevelDBMigrations.mm @@ -16,13 +16,12 @@ #include "Firestore/Source/Local/FSTLevelDBMigrations.h" +#include #include "leveldb/write_batch.h" #import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h" -#import "Firestore/Source/Local/FSTLevelDB.h" #import "Firestore/Source/Local/FSTLevelDBKey.h" #import "Firestore/Source/Local/FSTLevelDBQueryCache.h" -#import "Firestore/Source/Local/FSTWriteGroup.h" #import "Firestore/Source/Util/FSTAssert.h" NS_ASSUME_NONNULL_BEGIN @@ -30,6 +29,7 @@ NS_ASSUME_NONNULL_BEGIN // Current version of the schema defined in this file. static FSTLevelDBSchemaVersion kSchemaVersion = 2; +using firebase::firestore::local::LevelDbTransaction; using leveldb::DB; using leveldb::Iterator; using leveldb::Status; @@ -38,24 +38,24 @@ using leveldb::WriteOptions; /** * Ensures that the global singleton target metadata row exists in LevelDB. - * @param db The db in which to require the row. */ -static void EnsureTargetGlobal(std::shared_ptr db, FSTWriteGroup *group) { - FSTPBTargetGlobal *targetGlobal = [FSTLevelDBQueryCache readTargetMetadataFromDB:db]; +static void EnsureTargetGlobal(LevelDbTransaction *transaction) { + FSTPBTargetGlobal *targetGlobal = + [FSTLevelDBQueryCache readTargetMetadataWithTransaction:transaction]; if (!targetGlobal) { - [group setMessage:[FSTPBTargetGlobal message] forKey:[FSTLevelDBTargetGlobalKey key]]; + transaction->Put([FSTLevelDBTargetGlobalKey key], [FSTPBTargetGlobal message]); } } /** * Save the given version number as the current version of the schema of the database. * @param version The version to save - * @param group The transaction in which to save the new version number + * @param transaction The transaction in which to save the new version number */ -static void SaveVersion(FSTLevelDBSchemaVersion version, FSTWriteGroup *group) { +static void SaveVersion(FSTLevelDBSchemaVersion version, LevelDbTransaction *transaction) { std::string key = [FSTLevelDBVersionKey key]; std::string version_string = std::to_string(version); - [group setData:version_string forKey:key]; + transaction->Put(key, version_string); } /** @@ -65,30 +65,32 @@ static void SaveVersion(FSTLevelDBSchemaVersion version, FSTWriteGroup *group) { * * It assumes the metadata has already been written and is able to be read in this transaction. */ -static void AddTargetCount(std::shared_ptr db, FSTWriteGroup *group) { - std::unique_ptr it(db->NewIterator([FSTLevelDB standardReadOptions])); - Slice start_key = [FSTLevelDBTargetKey keyPrefix]; +static void AddTargetCount(LevelDbTransaction *transaction) { + std::unique_ptr it(transaction->NewIterator()); + std::string start_key = [FSTLevelDBTargetKey keyPrefix]; it->Seek(start_key); int32_t count = 0; - while (it->Valid() && it->key().starts_with(start_key)) { + while (it->Valid() && absl::StartsWith(it->key(), start_key)) { count++; it->Next(); } - FSTPBTargetGlobal *targetGlobal = [FSTLevelDBQueryCache readTargetMetadataFromDB:db]; + FSTPBTargetGlobal *targetGlobal = + [FSTLevelDBQueryCache readTargetMetadataWithTransaction:transaction]; FSTCAssert(targetGlobal != nil, @"We should have a metadata row as it was added in an earlier migration"); targetGlobal.targetCount = count; - [group setMessage:targetGlobal forKey:[FSTLevelDBTargetGlobalKey key]]; + transaction->Put([FSTLevelDBTargetGlobalKey key], targetGlobal); } @implementation FSTLevelDBMigrations -+ (FSTLevelDBSchemaVersion)schemaVersionForDB:(std::shared_ptr)db { ++ (FSTLevelDBSchemaVersion)schemaVersionWithTransaction: + (firebase::firestore::local::LevelDbTransaction *)transaction { std::string key = [FSTLevelDBVersionKey key]; std::string version_string; - Status status = db->Get([FSTLevelDB standardReadOptions], key, &version_string); + Status status = transaction->Get(key, &version_string); if (status.IsNotFound()) { return 0; } else { @@ -96,34 +98,24 @@ static void AddTargetCount(std::shared_ptr db, FSTWriteGroup *group) { } } -+ (void)runMigrationsOnDB:(std::shared_ptr)db { - FSTWriteGroup *group = [FSTWriteGroup groupWithAction:@"Migrations"]; - FSTLevelDBSchemaVersion currentVersion = [self schemaVersionForDB:db]; ++ (void)runMigrationsWithTransaction:(firebase::firestore::local::LevelDbTransaction *)transaction { + FSTLevelDBSchemaVersion currentVersion = [self schemaVersionWithTransaction:transaction]; // Each case in this switch statement intentionally falls through. This lets us // start at the current schema version and apply any migrations that have not yet // been applied, to bring us up to current, as defined by the kSchemaVersion constant. switch (currentVersion) { case 0: - EnsureTargetGlobal(db, group); + EnsureTargetGlobal(transaction); // Fallthrough case 1: - // We need to make sure we have metadata, since we're going to read and modify it - // in this migration. Commit the current transaction and start a new one. Since we're - // committing, we need to save a version. It's safe to save this one, if we crash - // after saving we'll resume from this step when we try to migrate. - SaveVersion(1, group); - [group writeToDB:db]; - group = [FSTWriteGroup groupWithAction:@"Migrations"]; - AddTargetCount(db, group); + // We're now guaranteed that the target global exists. We can safely add a count to it. + AddTargetCount(transaction); // Fallthrough default: if (currentVersion < kSchemaVersion) { - SaveVersion(kSchemaVersion, group); + SaveVersion(kSchemaVersion, transaction); } } - if (!group.isEmpty) { - [group writeToDB:db]; - } } @end diff --git a/Firestore/Source/Local/FSTLevelDBQueryCache.h b/Firestore/Source/Local/FSTLevelDBQueryCache.h index 1f6fbd4..d955b02 100644 --- a/Firestore/Source/Local/FSTLevelDBQueryCache.h +++ b/Firestore/Source/Local/FSTLevelDBQueryCache.h @@ -19,6 +19,7 @@ #include #import "Firestore/Source/Local/FSTQueryCache.h" +#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" #include "leveldb/db.h" @class FSTLocalSerializer; @@ -32,9 +33,16 @@ NS_ASSUME_NONNULL_BEGIN /** * Retrieves the global singleton metadata row from the given database, if it exists. + * TODO(gsoltis): remove this method once fully ported to transactions. */ + (nullable FSTPBTargetGlobal *)readTargetMetadataFromDB:(std::shared_ptr)db; +/** + * Retrieves the global singleton metadata row using the given transaction, if it exists. + */ ++ (nullable FSTPBTargetGlobal *)readTargetMetadataWithTransaction: + (firebase::firestore::local::LevelDbTransaction *)transaction; + - (instancetype)init NS_UNAVAILABLE; /** The garbage collector to notify about potential garbage keys. */ diff --git a/Firestore/Source/Local/FSTLevelDBQueryCache.mm b/Firestore/Source/Local/FSTLevelDBQueryCache.mm index fe1bf19..95b032e 100644 --- a/Firestore/Source/Local/FSTLevelDBQueryCache.mm +++ b/Firestore/Source/Local/FSTLevelDBQueryCache.mm @@ -31,6 +31,7 @@ NS_ASSUME_NONNULL_BEGIN +using firebase::firestore::local::LevelDbTransaction; using Firestore::StringView; using leveldb::DB; using leveldb::Iterator; @@ -59,6 +60,30 @@ using leveldb::WriteOptions; FSTSnapshotVersion *_lastRemoteSnapshotVersion; } ++ (nullable FSTPBTargetGlobal *)readTargetMetadataWithTransaction: + (firebase::firestore::local::LevelDbTransaction *)transaction { + std::string key = [FSTLevelDBTargetGlobalKey key]; + std::string value; + Status status = transaction->Get(key, &value); + if (status.IsNotFound()) { + return nil; + } else if (!status.ok()) { + FSTFail(@"metadataForKey: failed loading key %s with status: %s", key.c_str(), + status.ToString().c_str()); + } + + NSData *data = + [[NSData alloc] initWithBytesNoCopy:(void *)value.data() length:value.size() freeWhenDone:NO]; + + NSError *error; + FSTPBTargetGlobal *proto = [FSTPBTargetGlobal parseFromData:data error:&error]; + if (!proto) { + FSTFail(@"FSTPBTargetGlobal failed to parse: %@", error); + } + + return proto; +} + + (nullable FSTPBTargetGlobal *)readTargetMetadataFromDB:(std::shared_ptr)db { std::string key = [FSTLevelDBTargetGlobalKey key]; std::string value; diff --git a/Firestore/Source/Local/FSTWriteGroup.h b/Firestore/Source/Local/FSTWriteGroup.h index c21ff72..a554597 100644 --- a/Firestore/Source/Local/FSTWriteGroup.h +++ b/Firestore/Source/Local/FSTWriteGroup.h @@ -19,6 +19,7 @@ #include #include "Firestore/Source/Local/StringView.h" +#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" #include "leveldb/db.h" NS_ASSUME_NONNULL_BEGIN @@ -46,13 +47,15 @@ NS_ASSUME_NONNULL_BEGIN */ + (instancetype)groupWithAction:(NSString *)action; ++ (instancetype)groupWithAction:(NSString *)action + transaction:(firebase::firestore::local::LevelDbTransaction *)transaction; + - (instancetype)init __attribute__((unavailable("Use a static constructor instead"))); /** The action description assigned to this write group. */ @property(nonatomic, copy, readonly) NSString *action; -/** Returns YES if the write group has no messages in it. */ -- (BOOL)isEmpty; +@property(nonatomic, readonly) firebase::firestore::local::LevelDbTransaction *transaction; /** * Marks the given key for deletion. @@ -78,8 +81,8 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)setData:(Firestore::StringView)data forKey:(Firestore::StringView)key; -/** Writes the contents to the given LevelDB. */ -- (leveldb::Status)writeToDB:(std::shared_ptr)db; +/** Returns YES if the write group has no messages in it. */ +- (BOOL)isEmpty; @end diff --git a/Firestore/Source/Local/FSTWriteGroup.mm b/Firestore/Source/Local/FSTWriteGroup.mm index 80d09ca..9aee31a 100644 --- a/Firestore/Source/Local/FSTWriteGroup.mm +++ b/Firestore/Source/Local/FSTWriteGroup.mm @@ -16,13 +16,12 @@ #import "Firestore/Source/Local/FSTWriteGroup.h" -#import -#include #include #import "Firestore/Source/Local/FSTLevelDBKey.h" #import "Firestore/Source/Util/FSTAssert.h" +using firebase::firestore::local::LevelDbTransaction; using Firestore::StringView; using leveldb::DB; using leveldb::Slice; @@ -32,109 +31,62 @@ using leveldb::WriteOptions; NS_ASSUME_NONNULL_BEGIN -namespace Firestore { - -/** - * A WriteBatch::Handler implementation that extracts batch details from a leveldb::WriteBatch. - * This is used for describing a write batch primarily in log messages after a failure. - */ -class BatchDescription : public WriteBatch::Handler { - public: - BatchDescription() : ops_(0), size_(0), message_([NSMutableString string]) { - } - virtual ~BatchDescription(); - virtual void Put(const Slice &key, const Slice &value); - virtual void Delete(const Slice &key); - - // Converts the batch to a printable string description of it - NSString *ToString() const { - return [NSString - stringWithFormat:@"%d changes (%lu bytes):%@", ops_, (unsigned long)size_, message_]; - } - - // Disallow copies and moves - BatchDescription(const BatchDescription &) = delete; - BatchDescription &operator=(const BatchDescription &) = delete; - BatchDescription(BatchDescription &&) = delete; - BatchDescription &operator=(BatchDescription &&) = delete; - - private: - int ops_; - size_t size_; - NSMutableString *message_; -}; - -BatchDescription::~BatchDescription() { -} - -void BatchDescription::Put(const Slice &key, const Slice &value) { - ops_ += 1; - size_ += value.size(); - - [message_ appendFormat:@"\n - Put %@ (%lu bytes)", [FSTLevelDBKey descriptionForKey:key], - (unsigned long)value.size()]; -} - -void BatchDescription::Delete(const Slice &key) { - ops_ += 1; - - [message_ appendFormat:@"\n - Delete %@", [FSTLevelDBKey descriptionForKey:key]]; -} - -} // namespace Firestore - @interface FSTWriteGroup () - (instancetype)initWithAction:(NSString *)action NS_DESIGNATED_INITIALIZER; +- (instancetype)initWithAction:(NSString *)action transaction:(LevelDbTransaction *)transaction; @end @implementation FSTWriteGroup { int _changes; - WriteBatch _contents; } + (instancetype)groupWithAction:(NSString *)action { return [[FSTWriteGroup alloc] initWithAction:action]; } ++ (instancetype)groupWithAction:(NSString *)action + transaction:(firebase::firestore::local::LevelDbTransaction *)transaction { + return [[FSTWriteGroup alloc] initWithAction:action transaction:transaction]; +} + - (instancetype)initWithAction:(NSString *)action { if (self = [super init]) { _action = action; + _transaction = nullptr; } return self; } -- (NSString *)description { - Firestore::BatchDescription description; - Status status = _contents.Iterate(&description); - if (!status.ok()) { - FSTFail(@"Iterate over write batch should not fail"); +- (instancetype)initWithAction:(NSString *)action transaction:(LevelDbTransaction *)transaction { + if (self = [self initWithAction:action]) { + _transaction = transaction; } - return [NSString - stringWithFormat:@"", self.action, description.ToString()]; + return self; } - (void)removeMessageForKey:(StringView)key { - _contents.Delete(key); + FSTAssert(_transaction != nullptr, @"Using group without a transaction"); + Slice keySlice = key; + _transaction->Delete(keySlice.ToString()); _changes += 1; } - (void)setMessage:(GPBMessage *)message forKey:(StringView)key { - NSData *data = [message data]; - Slice value((const char *)data.bytes, data.length); - - _contents.Put(key, value); + FSTAssert(_transaction != nullptr, @"Using group without a transaction"); + Slice keySlice = key; + _transaction->Put(keySlice.ToString(), message); _changes += 1; } - (void)setData:(StringView)data forKey:(StringView)key { - _contents.Put(key, data); + FSTAssert(_transaction != nullptr, @"Using group without a transaction"); + Slice keySlice = key; + Slice valueSlice = data; + std::string value = valueSlice.ToString(); + _transaction->Put(keySlice.ToString(), value); _changes += 1; } -- (leveldb::Status)writeToDB:(std::shared_ptr)db { - return db->Write(leveldb::WriteOptions(), &_contents); -} - - (BOOL)isEmpty { return _changes == 0; } diff --git a/Firestore/Source/Local/FSTWriteGroupTracker.h b/Firestore/Source/Local/FSTWriteGroupTracker.h index bd26a46..c4651ab 100644 --- a/Firestore/Source/Local/FSTWriteGroupTracker.h +++ b/Firestore/Source/Local/FSTWriteGroupTracker.h @@ -15,6 +15,7 @@ */ #import +#import "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h" @class FSTWriteGroup; @@ -37,6 +38,10 @@ NS_ASSUME_NONNULL_BEGIN */ - (FSTWriteGroup *)startGroupWithAction:(NSString *)action; +- (FSTWriteGroup *)startGroupWithAction:(NSString *)action + transaction: + (firebase::firestore::local::LevelDbTransaction *)transaction; + /** Ends a group previously started with `startGroupWithAction`. */ - (void)endGroup:(FSTWriteGroup *)group; diff --git a/Firestore/Source/Local/FSTWriteGroupTracker.mm b/Firestore/Source/Local/FSTWriteGroupTracker.mm index 7e3bf60..2cb10bd 100644 --- a/Firestore/Source/Local/FSTWriteGroupTracker.mm +++ b/Firestore/Source/Local/FSTWriteGroupTracker.mm @@ -40,6 +40,17 @@ NS_ASSUME_NONNULL_BEGIN return self.activeGroup; } +- (FSTWriteGroup *)startGroupWithAction:(NSString *)action + transaction: + (firebase::firestore::local::LevelDbTransaction *)transaction { + // NOTE: We can relax this to allow nesting if/when we find we need it. + FSTAssert(!self.activeGroup, + @"Attempt to create write group (%@) while existing write group (%@) still active.", + action, self.activeGroup.action); + self.activeGroup = [FSTWriteGroup groupWithAction:action transaction:transaction]; + return self.activeGroup; +} + - (void)endGroup:(FSTWriteGroup *)group { FSTAssert(self.activeGroup == group, @"Attempted to end write group (%@) which is different from active group (%@)", -- cgit v1.2.3