Skip to content

Commit

Permalink
Resolves #138: Collection context in the cache should always include …
Browse files Browse the repository at this point in the history 
…indexes.

getUnboundCollectionContext() should always return context with indexes
included.

As this function plays with cache, its good to not play with how it
creates collection context. Instead client of this function can make a
copy and do anything with it. Otherwise, we will find interesting
inconsistencies in the metadata cache.
  • Loading branch information
@apkar
apkar committed Apr 4, 2019
1 parent 55fcbfd commit 68b5a83
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 25 deletions.
2 changes: 2 additions & 0 deletions src/Constants.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const uint64_t INDEX_KEY_LENGTH_LIMIT = (uint64_t)1e4;
const uint64_t FDB_KEY_LENGTH_LIMIT = (uint64_t)1e4;
const uint64_t FDB_VALUE_LENGTH_LIMIT = (uint64_t)1e5;

const uint64_t METADATA_CACHE_SIZE = (uint64_t)100;

const std::string METADATA = "metadata";
const std::string VERSION_KEY = "version";
const std::string INDICES_KEY = "indices";
Expand Down
3 changes: 3 additions & 0 deletions src/Constants.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ extern const uint64_t INDEX_KEY_LENGTH_LIMIT; // This is set to 10K bytes, inste
extern const uint64_t FDB_KEY_LENGTH_LIMIT;
extern const uint64_t FDB_VALUE_LENGTH_LIMIT;

// Size of metadata cache in entries, number of collections
extern const uint64_t METADATA_CACHE_SIZE;

// KVS DocLayer internal keys
extern const std::string METADATA;
extern const std::string VERSION_KEY;
Expand Down
2 changes: 1 addition & 1 deletion src/ExtMsg.actor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,7 @@ ACTOR Future<WriteCmdResult> doDeleteCmd(Namespace ns,
// If collection not found then just return success from here.
try {
Reference<UnboundCollectionContext> _cx =
wait(ec->mm->getUnboundCollectionContext(dtr, ns, false, true, false));
wait(ec->mm->getUnboundCollectionContext(dtr, ns, false, false));
cx = _cx;
} catch (Error& e) {
if (e.code() == error_code_collection_not_found)
Expand Down
37 changes: 14 additions & 23 deletions src/MetadataManager.actor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> co
Namespace ns,
Reference<DocTransaction> tr,
DocumentLayer* docLayer,
bool includeIndex,
bool createCollectionIfAbsent) {
try {
// The initial set of directory reads take place in a separate transaction with the same read version as `tr'.
Expand All @@ -112,20 +111,15 @@ ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> co
state Reference<UnboundCollectionContext> cx =
Reference<UnboundCollectionContext>(new UnboundCollectionContext(collectionDirectory, metadataDirectory));

// Only include existing indexes into the context when it's NOT building a new index.
// When it's building a new index, it's unnecessary and inefficient to pass each recorded returned by a
// TableScan through the existing indexes.
if (includeIndex) {
state Reference<UnboundCollectionContext> indexCx = Reference<UnboundCollectionContext>(
new UnboundCollectionContext(indexDirectory, Reference<DirectorySubspace>()));
state Reference<Plan> indexesPlan = getIndexesForCollectionPlan(indexCx, ns);
std::vector<bson::BSONObj> allIndexes = wait(getIndexesTransactionally(indexesPlan, tr));
state Reference<UnboundCollectionContext> indexCx = Reference<UnboundCollectionContext>(
new UnboundCollectionContext(indexDirectory, Reference<DirectorySubspace>()));
state Reference<Plan> indexesPlan = getIndexesForCollectionPlan(indexCx, ns);
std::vector<bson::BSONObj> allIndexes = wait(getIndexesTransactionally(indexesPlan, tr));

for (const auto& indexObj : allIndexes) {
IndexInfo index = MetadataManager::indexInfoFromObj(indexObj, cx);
if (index.status != IndexInfo::IndexStatus::INVALID) {
cx->addIndex(index);
}
for (const auto& indexObj : allIndexes) {
IndexInfo index = MetadataManager::indexInfoFromObj(indexObj, cx);
if (index.status != IndexInfo::IndexStatus::INVALID) {
cx->addIndex(index);
}
}

Expand Down Expand Up @@ -172,16 +166,15 @@ ACTOR static Future<std::pair<Reference<UnboundCollectionContext>, uint64_t>> co
ACTOR static Future<Reference<UnboundCollectionContext>> assembleCollectionContext(Reference<DocTransaction> tr,
Namespace ns,
Reference<MetadataManager> self,
bool includeIndex,
bool createCollectionIfAbsent) {
if (self->contexts.size() > 100)
if (self->contexts.size() > DocLayerConstants::METADATA_CACHE_SIZE)
self->contexts.clear();

auto match = self->contexts.find(ns);

if (match == self->contexts.end()) {
std::pair<Reference<UnboundCollectionContext>, uint64_t> unboundPair =
wait(constructContext(ns, tr, self->docLayer, includeIndex, createCollectionIfAbsent));
wait(constructContext(ns, tr, self->docLayer, createCollectionIfAbsent));

// Here and below don't pollute the cache if we just created the directory, since this transaction might
// not commit.
Expand All @@ -204,7 +197,7 @@ ACTOR static Future<Reference<UnboundCollectionContext>> assembleCollectionConte
uint64_t version = wait(getMetadataVersion(tr, oldUnbound->metadataDirectory));
if (version != oldVersion) {
std::pair<Reference<UnboundCollectionContext>, uint64_t> unboundPair =
wait(constructContext(ns, tr, self->docLayer, includeIndex, createCollectionIfAbsent));
wait(constructContext(ns, tr, self->docLayer, createCollectionIfAbsent));
if (unboundPair.second != -1) {
// Create the iterator again instead of making the previous value state, because the map could have
// changed during the previous wait. Either way, replace it with ours (can no longer optimize this by
Expand All @@ -231,19 +224,17 @@ Future<Reference<UnboundCollectionContext>> MetadataManager::getUnboundCollectio
Reference<DocTransaction> tr,
Namespace const& ns,
bool allowSystemNamespace,
bool includeIndex,
bool createCollectionIfAbsent) {
if (!allowSystemNamespace && startsWith(ns.second.c_str(), "system."))
throw write_system_namespace();
return assembleCollectionContext(tr, ns, Reference<MetadataManager>::addRef(this), includeIndex,
createCollectionIfAbsent);
return assembleCollectionContext(tr, ns, Reference<MetadataManager>::addRef(this), createCollectionIfAbsent);
}

Future<Reference<UnboundCollectionContext>> MetadataManager::refreshUnboundCollectionContext(
Reference<UnboundCollectionContext> cx,
Reference<DocTransaction> tr) {
return assembleCollectionContext(tr, std::make_pair(cx->databaseName(), cx->collectionName()),
Reference<MetadataManager>::addRef(this), false, false);
Reference<MetadataManager>::addRef(this), false);
}

ACTOR static Future<Void> buildIndex_impl(bson::BSONObj indexObj,
Expand All @@ -254,7 +245,7 @@ ACTOR static Future<Void> buildIndex_impl(bson::BSONObj indexObj,
state IndexInfo info;
try {
state Reference<DocTransaction> tr = ec->getOperationTransaction();
state Reference<UnboundCollectionContext> mcx = wait(ec->mm->getUnboundCollectionContext(tr, ns, false, false));
state Reference<UnboundCollectionContext> mcx = wait(ec->mm->getUnboundCollectionContext(tr, ns, false));
info = MetadataManager::indexInfoFromObj(indexObj, mcx);
info.status = IndexInfo::IndexStatus::BUILDING;
info.buildId = build_id;
Expand Down
1 change: 0 additions & 1 deletion src/MetadataManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ struct MetadataManager : ReferenceCounted<MetadataManager>, NonCopyable {
Future<Reference<UnboundCollectionContext>> getUnboundCollectionContext(Reference<DocTransaction> tr,
Namespace const& ns,
bool allowSystemNamespace = false,
bool includeIndex = true,
bool createCollectionIfAbsent = true);
Future<Reference<UnboundCollectionContext>> refreshUnboundCollectionContext(Reference<UnboundCollectionContext> cx,
Reference<DocTransaction> tr);
Expand Down

0 comments on commit 68b5a83

Please sign in to comment.