Skip to content

The consistency check should retry if it couldn't find all the commit proxies when getting key server locations #9744

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.

Sign up for GitHub

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

Merged
sfc-gh-abeamon merged 2 commits into from
Mar 17, 2023
Merged

The consistency check should retry if it couldn't find all the commit proxies when getting key server locations #9744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc2bd78
The consistency check should retry if it couldn't find all the commit…
sfc-gh-abeamon Mar 17, 2023
fe5d092
Remove doEmptyCommit function
sfc-gh-abeamon Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions fdbclient/include/fdbclient/ConsistencyScanInterface.actor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ ACTOR Future<bool> getKeyServers(
Promise<std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>>> keyServersPromise,
KeyRangeRef kr,
bool performQuiescentChecks,
bool failureIsError,
bool* success);
ACTOR Future<bool> getKeyLocations(Database cx,
std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>> shards,
Expand Down
6 changes: 4 additions & 2 deletions fdbserver/ConsistencyScan.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ ACTOR Future<bool> getKeyServers(
Promise<std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>>> keyServersPromise,
KeyRangeRef kr,
bool performQuiescentChecks,
bool failureIsError,
bool* success) {
state std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>> keyServers;

Expand Down Expand Up @@ -134,7 +135,7 @@ ACTOR Future<bool> getKeyServers(
TraceEvent("ConsistencyCheck_CommitProxyUnavailable")
.error(shards.getError())
.detail("CommitProxyID", commitProxyInfo->getId(i));
testFailure("Commit proxy unavailable", performQuiescentChecks, success, true);
testFailure("Commit proxy unavailable", performQuiescentChecks, success, failureIsError);
return false;
}

Expand Down Expand Up @@ -979,7 +980,8 @@ ACTOR Future<Void> runDataValidationCheck(ConsistencyScanData* self) {
// Get a list of key servers; verify that the TLogs and master all agree about who the key servers are
state Promise<std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>>> keyServerPromise;
state std::map<UID, StorageServerInterface> tssMapping;
bool keyServerResult = wait(getKeyServers(self->db, keyServerPromise, keyServersKeys, false, &self->success));
bool keyServerResult =
wait(getKeyServers(self->db, keyServerPromise, keyServersKeys, false, false, &self->success));
if (keyServerResult) {
state std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>> keyServers =
keyServerPromise.getFuture().get();
Expand Down
37 changes: 8 additions & 29 deletions fdbserver/workloads/ConsistencyCheck.actor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,12 @@ struct ConsistencyCheckWorkload : TestWorkload {

// Get a list of key servers; verify that the TLogs and master all agree about who the key servers are
state Promise<std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>>> keyServerPromise;
bool keyServerResult = wait(
getKeyServers(cx, keyServerPromise, keyServersKeys, self->performQuiescentChecks, &self->success));
bool keyServerResult = wait(getKeyServers(cx,
keyServerPromise,
keyServersKeys,
self->performQuiescentChecks,
self->failureIsError,
&self->success));
if (keyServerResult) {
state std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>> keyServers =
keyServerPromise.getFuture().get();
Expand Down Expand Up @@ -797,8 +801,8 @@ struct ConsistencyCheckWorkload : TestWorkload {
bool removePrefix) {
// get shards paired with corresponding storage servers
state Promise<std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>>> keyServerPromise;
bool keyServerResult =
wait(getKeyServers(cx, keyServerPromise, range, self->performQuiescentChecks, &self->success));
bool keyServerResult = wait(getKeyServers(
cx, keyServerPromise, range, self->performQuiescentChecks, self->failureIsError, &self->success));
if (!keyServerResult)
return false;
state std::vector<std::pair<KeyRange, std::vector<StorageServerInterface>>> shards =
Expand Down Expand Up @@ -1041,23 +1045,6 @@ struct ConsistencyCheckWorkload : TestWorkload {
return true;
}

// Run an empty commit through the system.
ACTOR static Future<Void> doEmptyCommit(Database cx) {
state Transaction tr(cx);
loop {
try {
tr.setOption(FDBTransactionOptions::PRIORITY_SYSTEM_IMMEDIATE);
tr.setOption(FDBTransactionOptions::LOCK_AWARE);
wait(::success(tr.getReadVersion()));
tr.makeSelfConflicting();
wait(tr.commit());
return Void();
} catch (Error& e) {
wait(tr.onError(e));
}
}
}

ACTOR Future<bool> checkForExtraDataStores(Database cx, ConsistencyCheckWorkload* self) {
state std::vector<WorkerDetails> workers = wait(getWorkers(self->dbInfo));
state std::vector<StorageServerInterface> storageServers = wait(getStorageServers(cx));
Expand Down Expand Up @@ -1160,14 +1147,6 @@ struct ConsistencyCheckWorkload : TestWorkload {
}

if (foundExtraDataStore) {
// Let the cluster fully recover after rebooting/killing storage servers with extra stores.
//
// This requires an end-to-end comitting transaction to ensure recovery has started in case
// any stateless processes, like the commit proxy, were killed.
wait(::success(doEmptyCommit(cx)));

@sfc-gh-dadkins sfc-gh-dadkins Mar 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting rid of this hack. You can also remove the doEmptyCommit function above, too. I only added it for this one call.

while (self->dbInfo->get().recoveryState != RecoveryState::FULLY_RECOVERED) {
wait(self->dbInfo->onChange());
}
self->testFailure("Extra data stores present on workers");
return false;
}
Expand Down