Recovery Optimization #1713

mengranwo · 2019-06-17T17:26:03Z

Sorry, had to open a new PR on this issue
Issue we(Wavefront) have:
#53

Solution:
Based on branch mater
During recovery process for memory storage engine, we’ll check periodically whether we can safely remove this storage engine (by calling canRemoveStorageServer() method). If the cluster is healthy then abort recovery and join in as an empty node. Otherwise, keep going until it’s done.

…e engine), if healthy, delete data files and join as new

alexmiller-apple

You appear to have used spaces for indentation, which should be converted to tabs.

I think you can also just run git clang-format to fix this as well.

alexmiller-apple · 2019-06-17T20:26:59Z

@fdb-tools, ok to test

20190617-132559-mengranwo-49c193ec9d88b498

mpilman · 2019-06-17T22:59:08Z

fdbserver/storageserver.actor.cpp

 		return false;
 }

+ACTOR Future<bool> memoryStoreRecover(IKeyValueStore* store, Reference<ClusterConnectionFile> connFile, UID id)


I would suggest to return Future<Void> here as this actor either returns true or doesn't return at all...

The reason why i had the return value as Future<bool> is that i wanted to check the storage engine type at the beginning, if the type != memory, i would return immediately and do nothing.

if(store->getType() != KeyValueStoreType::MEMORY || connFile.getPtr() == nullptr) { return false; }

this part, to be more specifically

You could also return Never() in this case... It would simplify your code quite a bit, but I am not going to insist

mpilman · 2019-06-17T22:59:40Z

fdbserver/storageserver.actor.cpp

-		wait(self.storage.commit()); //after a rollback there might be uncommitted changes.
+
+		state Future<bool> dispose = memoryStoreRecover (persistentData, connFile, self.thisServerID);
+        wait(self.storage.init());


Indentation seems to be wrong here.

mpilman · 2019-06-17T23:03:52Z

fdbserver/storageserver.actor.cpp

+		//after a rollback there might be uncommitted changes.
+		//for memory storage engine type, wait until recovery is done before commit
+		state Future<Void> committed = self.storage.commit();
+		wait(success(dispose) || success(committed));


if dispose would return a Future<Void> you could instead use a choose-when construct here (and you can get rid of the two state variables):

choose { when (wait(self.storage.commit())) {} when (wait(memoryStoreRecover (persistentData, connFile, self.thisServerID))) { TraceEvent("DisposeStorageServer", self.thisServerID); throw worker_removed(); } }

If you do this you also don't need to explicitly cancel the dispose Future.

i see. However, one of my concern is, if type != memory, the method memoryStoreRecover() would immediately return Never(), but in this case, we don't want to throw worker_removed(); right?

What is the difference between Never() and Void() then ?

Never mind, figure it out.
push the changes into branch, thank you!

alexmiller-apple · 2019-06-18T00:09:22Z

20190617-132559-mengranwo-49c193ec9d88b498 branch='recovery-optimization' compressed=True data_size=12929949 duration=2869496 ended=100233 fail_fast=10 max_runs=100000 pass=100000 pr='1713' priority=100 sanity=False started=100234 timeout=5400 username='mengranwo'

Correctness approves.

alexmiller-apple · 2019-06-21T00:56:32Z

fdbserver/storageserver.actor.cpp

+			//for memory storage engine type, wait until recovery is done before commit
+			wait(self.storage.commit()) {}
+
+			wait(memoryStoreRecover (persistentData, connFile, self.thisServerID)) {


You're missing the whens

mpilman

This now looks good to me - but I would like the code-formatting to be a bit more consistent

mpilman · 2019-06-21T17:04:27Z

fdbserver/storageserver.actor.cpp


+ACTOR Future<Void> memoryStoreRecover(IKeyValueStore* store, Reference<ClusterConnectionFile> connFile, UID id)
+{
+    if(store->getType() != KeyValueStoreType::MEMORY || connFile.getPtr() == nullptr) {


This if-block has a different indentation then the rest of the function.

run git clang-format on storageserver.cpp

mengranwo · 2019-06-27T01:15:25Z

@etschannen
Evan, if you have time, can you take a look at this PR, we talked about the high-level design months ago. Would love to get this one merged into the master branch

Thank you!

etschannen · 2019-06-28T00:46:58Z

fdbserver/storageserver.actor.cpp

+				tr.reset();
+				TraceEvent("RemoveStorageServerRetrying").detail("Count", noCanRemoveCount++).detail("ServerID", id).detail("CanRemove", canRemove);
+			} else {
+				wait(tr.commit());


Nothing is being written, so there is no need to commit the transaction

I was wondering what's the protocol here.
For the read & write transaction, we need to commit at the end.
But for the read-only transaction, we can skip this step? (To avoid the overhead?)

etschannen · 2019-06-28T00:48:36Z

fdbserver/storageserver.actor.cpp

+			try {
+			    wait( tr.onError(e));
+			} catch (Error& e2) {
+			    return Never();


what non-retryable errors is this second try catch expected to catch?

get rid of the try & block
resolved

etschannen · 2019-06-28T00:51:22Z

Sorry for taking so long to review this. I posted some very minor code cleanup comments, and will merge after you have looked at them.

mengranwo added 3 commits June 16, 2019 16:20

checking cluster is healthy or not during recovery process(for storag…

69dbb19

…e engine), if healthy, delete data files and join as new

fix syntax error, pass compile

0102820

add try catch block around tx.onerror()

3fa8fa3

alexmiller-apple reviewed Jun 17, 2019

View reviewed changes

hgray1 requested review from alexmiller-apple and mpilman June 17, 2019 20:26

hgray1 assigned etschannen Jun 17, 2019

hgray1 requested a review from etschannen June 17, 2019 20:28

fix format issue

628709a

mpilman suggested changes Jun 17, 2019

View reviewed changes

address cr comments:

b46cd07

alexmiller-apple reviewed Jun 21, 2019

View reviewed changes

fix compiling error

c9e4590

mpilman suggested changes Jun 21, 2019

View reviewed changes

style formatting

cb11287

mpilman approved these changes Jun 22, 2019

View reviewed changes

etschannen reviewed Jun 28, 2019

View reviewed changes

Address pr comments, remove wait(tr.commit()) for read-only txn

7c7ae03

alexmiller-apple merged commit e54eedf into apple:master Jul 1, 2019

mengranwo deleted the mengran/recovery-optimization branch July 7, 2019 23:40

Recovery Optimization #1713

Recovery Optimization #1713

Uh oh!

Conversation

mengranwo commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexmiller-apple left a comment

Choose a reason for hiding this comment

Uh oh!

alexmiller-apple commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengranwo Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengranwo Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengranwo Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexmiller-apple commented Jun 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpilman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengranwo commented Jun 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengranwo Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etschannen commented Jun 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mengranwo commented Jun 17, 2019 •

edited

Loading

mengranwo Jun 17, 2019 •

edited

Loading

mengranwo Jun 17, 2019 •

edited

Loading

mengranwo Jun 21, 2019 •

edited

Loading

mengranwo Jun 30, 2019 •

edited

Loading