New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolves #1430: Support non-idempotent target indexes while indexing by index #1904
Resolves #1430: Support non-idempotent target indexes while indexing by index #1904
Conversation
…le indexing by index This adds support for building a non-idempotent target index when indexing from a different source index. This works in a manner that is analogous to the way that this operation works for non-idempotent indexes built by a record scan, except that as the range set contains ranges of index entries from the source index, the maintainer needs to be updated to: (1) check the indexing type stamp and (2) modify the range set check to use the index key instead of the primary key. Then there are some updates to the indexers to adjust logic that assumed the target index type would always be idempotent. This resolves FoundationDB#1430.
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
IndexBuildProto.IndexBuildIndexingStamp indexingTypeStamp = getIndexingTypeStamp(store); | ||
|
||
return forEachTargetIndex(index -> setIndexingTypeOrThrow(store, continuedBuild, transaction, index, indexingTypeStamp)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this code change all the indexes' typestamp with a single transaction? (to avoid changing few indexes but not all..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the transactionality of the operation here hasn't changed. The only thing that changed was that the Transaction
object is now no longer passed separately from the store, which I made because that way, it is clearer that the store and transaction are linked (i.e., that someone wasn't doing something sneaky like passing a store that was created with a different transaction)
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
One thing that hasn't been addressed by this PR (yet): how should this be rolled out? The main concern here with roll out is that if someone starts an index-from-index build on a non-idempotent index, but there are still instances accessing the same record store that are running an older version of the code, then the record update won't check the correct values in the range set, which can result in index corruption. Note that the data written by builds are unchanged, so any ongoing index build can continue |
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
…s that sets maintenance filter and format version
…se a new store format version to prevent data corruption
1c050bf
to
df488e7
Compare
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -3499,6 +3492,32 @@ public CompletableFuture<IndexBuildState> getIndexBuildStateAsync(Index index) { | |||
return IndexBuildState.loadIndexBuildStateAsync(this, index); | |||
} | |||
|
|||
@API(API.Status.INTERNAL) | |||
@Nonnull | |||
public CompletableFuture<IndexBuildProto.IndexBuildIndexingStamp> loadIndexBuildStampAsync(Index index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would loadIndexingTypeStamp
be more compatible with the rest of the code?
(+ changing all indexBuildStamp
to IndexingTypeStamp
below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Changed.
* Some sanity checks will be performed, but it is the caller's responsibility to verify that this | ||
* source-index covers <em>all</em> the relevant records for the target-index. Also, note that | ||
* if the {@linkplain OnlineIndexer.Builder#setIndex(Index) target index} is not idempotent, | ||
* the index build will not be executed using the given source index unless the store's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be: "the indexer will throw an exception and may fall back to by record
scan unless..." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a clarification as to what happens in that case (that there is a fallback). I believe the old documentation is still correct (the build won't be executed using the source index in the case specified), but I've added more detail about what does happen.
} | ||
|
||
private <M extends Message> CompletableFuture<Void> updateWriteOnlyByRecords(@Nullable final FDBIndexableRecord<M> oldRecord, @Nullable final FDBIndexableRecord<M> newRecord) { | ||
// Check if the record has been built be checking its primary key in the range set. Update the index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"be" = "by"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return addedRangeWithKey(oldEntryKey) | ||
.thenCompose(oldInRange -> oldInRange ? update(oldRecord, null) : AsyncUtil.DONE) | ||
.thenCompose(ignore -> addedRangeWithKey(newEntryKey)) | ||
.thenCompose(newInRange -> newInRange ? update(null, newRecord) : AsyncUtil.DONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would allOf
make sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of. Index maintainers are not thread safe when executing on the same subspace, so the index updates need to be in serial. However, the range set checks should be able to be done concurrently.
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
// respond appropriately | ||
if (store.getFormatVersion() < FDBRecordStore.CHECK_INDEX_BUILD_TYPE_DURING_UPDATE_FORMAT_VERSION) { | ||
validateOrThrowEx(maintainer.isIdempotent(), "target index is not idempotent"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would
validateOrThrowEx(maintainer.isIdempotent() ||
(store.getFormatVersion() >= FDBRecordStore.CHECK_INDEX_BUILD_TYPE_DURING_UPDATE_FORMAT_VERSION),
"target index is not idempotent")
be a little more elegant?
(not a big deal..)
try (OnlineIndexer indexBuilder = newIndexerBuilder() | ||
.addTargetIndex(tgtIndex) | ||
// Set a format version on the store that does not allow index-from-index builds | ||
// Because the store already has this format version, though it should be allowed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that I understand the second line of the comment.
Did you mean to mention that the indexing fallback to "by records" scan?
(fallsback? fallbacks? falls back?)
|
||
// The current code can read and write up to the format version below | ||
public static final int MAX_SUPPORTED_FORMAT_VERSION = READABLE_UNIQUE_PENDING_FORMAT_VERSION; | ||
public static final int MAX_SUPPORTED_FORMAT_VERSION = CHECK_INDEX_BUILD_TYPE_DURING_UPDATE_FORMAT_VERSION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside the scope of this PR, and it would break backwards compatibility, but the naming of these is confusing, and could probably benefit from being moved to a separate class, then it would be FDBRecordStoreFormatVersions.CHECK_INDEX_BUILD_TYPE_DURING_UPDATE
/** | ||
* Update the associated index for a changed record while the index is in | ||
* {@link com.apple.foundationdb.record.IndexState#WRITE_ONLY} mode. For most indexes, this should do the | ||
* same thing that a normal update does, but if the index is not {@linkplain #isIdempotent() idempotent}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, at some point, the existence of updateWhileWriteOnly
could allow us to remove isIdempontent
, although we may want to expand the documentation to more clearly describe the behavior that an implementor needs to handle for both update
and updateWhileWriteOnly
* @return a future that is complete when the index update is done | ||
*/ | ||
@Nonnull | ||
public abstract <M extends Message> CompletableFuture<Void> updateWhileWriteOnly(@Nullable FDBIndexableRecord<M> oldRecord, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall what the stance is, but this is a breaking change for implementors of IndexMaintainer
. Should this have a default implementation that is what we previously used to do in the record store, and then, comment that the default implementation will be removed in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we ever came down on a super firm stance here, but in this case, I think we want to encourage an adopter who is directly extending IndexMaintainer
to take a look at their implementation to see if they need to do anything different for WRITE_ONLY
indexes, now that the option is available, and it's better that that be the case than
@API(API.Status.INTERNAL) | ||
@Nonnull | ||
public CompletableFuture<IndexBuildProto.IndexBuildIndexingStamp> loadIndexingTypeStampAsync(Index index) { | ||
byte[] stampKey = IndexingBase.indexBuildTypeSubspace(this, index).pack(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit weird that IndexingBase
continues to manage what the subspace is, but not how to write to it.
// index key expression always returns a single value. In this case, the record is excluded from the index | ||
return null; | ||
} else if (entries.size() != 1) { | ||
throw new RecordCoreException("index produced incorrect number of entries for use as source index"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this restriction apply for idempotent indexes, or is this a new restriction for non-idempotent indexes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a new restriction: there was already a check in the indexer to make sure the index did not create duplicates. However, it is more important for non-idempotent target indexes.
Along those lines, there are actually two codepaths, a transactional build and a non-transactional build, and it turns out this check was missed in the transactional case. I've added the check in the the other case, which also added a few other missed checks.
.setRecordsPerSecond(OnlineIndexer.DEFAULT_RECORDS_PER_SECOND * 100) | ||
.build()) { | ||
CompletableFuture<?> buildFuture = indexer.buildIndexAsync(true); | ||
recordsAfter = updater.update(recordsBefore, otherRecordsBefore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's definitely a risk that these won't really end up testing concurrent problems, at least with the numbers in the test, but I don't have a good solution, other than make a very large test that has a ton of records, takes hours to build, and randomly does all sorts of mutations while doing so, and then asserts that the result is correct. But, the record layer doesn't really have the infrastructure to do that kind of testing.
@Nonnull | ||
@Override | ||
public <M extends Message> CompletableFuture<Void> updateWhileWriteOnly(@Nullable final FDBIndexableRecord<M> oldRecord, @Nullable final FDBIndexableRecord<M> newRecord) { | ||
return AsyncUtil.DONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just call update
? or return a failed exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had this return DONE
rather than throw an error because this index maintainer always returns false
to isIdempotent
, so this is behavior preserving. I didn't try throwing an error, but IIRC, this maintainer is used in some somewhat delicate tests, so it could cause some tests to break, I think
if (!MoreAsyncUtil.isCompletedNormally(future)) { | ||
futures.add(future); | ||
} | ||
if (isIndexWriteOnly(index)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other calls to update
, do any of those need to be updated to do this check? They weren't doing the previous check, so they may have already been broken for non-idempotent use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that seems to be the case. The other calls are:
- Within the
TextIndexMaintainer
, which will only get called on (idempotent) text indexes, so we're fine - Within the indexer, which wants to update the index maintainer unconditionally
- Within
FDBRecordStore::runSyntheticMaintainers
, which I'm less certain about, but that already was in a bit of a suspect state
So nothing seems to be updated in response to this, but maybe we should revisit this after this is in
There are a few cleanup/comment/refactoring things that I will try and get into a follow up, but I've focused on addressing things that might affect correctness in this PR right now, but I don't want to delay this too much given that without it, affected index builds could be delayed significantly |
SonarCloud Quality Gate failed. |
Result of fdb-record-layer-pr-proto2 on Linux CentOS 7
|
Result of fdb-record-layer-pr-proto3 on Linux CentOS 7
|
…by index (#1904) * Resolves #1430: Support non-idempotent target indexes while indexing by index This adds support for building a non-idempotent target index when indexing from a different source index. This works in a manner that is analogous to the way that this operation works for non-idempotent indexes built by a record scan, except that as the range set contains ranges of index entries from the source index, the maintainer needs to be updated to: (1) check the indexing type stamp and (2) modify the range set check to use the index key instead of the primary key. Then there are some updates to the indexers to adjust logic that assumed the target index type would always be idempotent. This resolves #1430.
This adds support for building a non-idempotent target index when indexing from a different source index. This works in a manner that is analogous to the way that this operation works for non-idempotent indexes built by a record scan, except that as the range set contains ranges of index entries from the source index, the maintainer needs to be updated to: (1) check the indexing type stamp and (2) modify the range set check to use the index key instead of the primary key. Then there are some updates to the indexers to adjust logic that assumed the target index type would always be idempotent.
I played around with the tests added in this PR, and I've validated that if I inject a few bugs into the code (like checking the wrong value in the range set) that they fail, so I'm fairly confident in their ability to cover the interesting scenarios. The exception there would be
deleteRecordsWhere
, which is not currently validated in any of the indexing tests, though which I believe to be correctly handled.This resolves #1430.