transaction: Resilient Txn Engine #1268

oshankkumar · 2018-10-08T13:08:30Z

The transaction engine executes the given transaction
across the cluster.The engine is designed to make use
of etcd as the means of communication between peers.

Please refer Design Document #1003

Signed-off-by: Oshank Kumar okumar@redhat.com

atinmu · 2018-10-08T14:49:39Z

@oshankkumar Can we please have 'Fixes' or 'Updates' tag in the commit message?

atinmu · 2018-10-08T14:54:00Z

@oshankkumar It might be worth to mention which all parts of #919 are covered here and what's pending. This will give us a good heads up for the reviewers.

oshankkumar · 2018-10-11T09:59:52Z

Task List

Transaction Engine
Creating and running a transaction.
Synchronized step execution
Modify global data structures
Handling Failure Txn
Cleanup Leader
Handling peer restart during transaction (partially Done)

kshlm

This is great. Looks good overall from the logic and structuring. I have several general comments about style. In addition, please try to add more comment through the code. Especially for the unexported helper functions, this will make understanding the code much easier in the future.

kshlm · 2018-10-30T11:37:10Z

glusterd2/transaction/context.go

@@ -33,29 +33,32 @@ type TxnCtx interface {
 	// Logger returns the Logrus logger associated with the context
 	Logger() log.FieldLogger

-	// commit writes all locally cached keys and values into the store using
+	// Commit writes all locally cached keys and values into the store using


Just to be clear here. You're exporting this and the other types and functions in this package, because of the new package, right? Once we refactor out the old package, these can go back to being unexported.

Yes Kaushal, I have made few things exported because I wanted to use in new package

kshlm · 2018-10-30T11:42:05Z

glusterd2/transactionv2/cleanuphandler/cleanup_handler.go

+
+const (
+	leaderKey        = "leader"
+	cleanupTimerDur  = time.Second * 5


Let's keep the timer durations for longer. I'm looking at this in minutes rather than seconds.

kshlm · 2018-10-30T11:50:00Z

glusterd2/transactionv2/cleanuphandler/cleanup_handler.go

+
+// StartElecting triggers a new election after every `electionTimerDur`.
+// If it succeeded then it assumes the leader role and returns
+func (c *CleanupHandler) StartElecting() {


For leader election, I'd prefer if we use github.com/coreos/etcd/clientv3/concurrency.Election, instead of rolling our own.

The concurrency package is already used elsewhere in the store package for concurrency.Session.

kshlm · 2018-10-30T11:51:32Z

glusterd2/transactionv2/engine.go

+
+// TxnEngine executes the given transaction across the cluster.
+// It makes use of etcd as the means of communication between nodes.
+type TxnEngine struct {


Just a comment on the naming. You can have this be just Engine. The rest of the names with txn prefix can also do without it. For things like the StepManager you can have the prefix.

kshlm · 2018-10-30T11:52:02Z

glusterd2/transactionv2/engine.go

+)
+
+// TransactionEngine is responsible for executing newly added txn
+var TransactionEngine *TxnEngine


Do you plan to use this variable outside this package? I don't think you really need to export this.

kshlm · 2018-10-30T12:18:57Z

glusterd2/transactionv2/engine.go

+		}
+
+		if err := txnEng.stepManager.RunStep(ctx, step, txn.Ctx); err != nil {
+			txn.Ctx.Logger().WithError(err).Errorf("failed in executing step %+v", step)


Avoid using log.*f variants of the log functions. Instead add context to the log message using WithField or WithFields.

kshlm · 2018-10-30T12:21:18Z

glusterd2/transactionv2/engine.go

+				txn.Ctx.Logger().WithError(err).Errorf("encounter an error in synchronizing txn step %+v", step)
+				return err
+			}
+			txn.Ctx.Logger().Info("transaction got synchronized")


This should be a debug log. Other than transaction started and finished logs, everything else should be debug. If needed any error logs should contain the full context to help identify the error cause.

oshankkumar · 2018-11-05T13:51:28Z

retest this please

Madhu-1 · 2018-11-05T13:54:38Z

@nigelbabu we have a CI failure, any hint why its failing?

13:16:00 [gluster_glusterd2] $ /bin/sh -xe /tmp/jenkins82820089879639684.sh
13:16:00 + SSID_FILE=/home/gluster/workspace/gluster_glusterd2/cico-ssid
13:16:00 ++ cat /home/gluster/workspace/gluster_glusterd2/cico-ssid
13:16:00 cat: /home/gluster/workspace/gluster_glusterd2/cico-ssid: No such file or directory

kshlm

Looks good. I'll approve once I test this out.

atinmu · 2018-11-16T04:21:47Z

@kshlm Did you get a chance to test this? Can we please expedite to get this PR in?

kshlm

Sorry for the delay in my response here. I tested this out, and it mostly works. Still though, the existing transactions need to be updated to sync the required steps. This mostly is having the 'store' steps be synchronized. There are also a few other steps that would require synchronization, like the barrier activate step in snapshot. Please complete this before we can move on to merging.

kshlm · 2018-11-20T13:26:18Z

glusterd2/transaction/lock.go

 		return ErrLockExists
 	}

-	logger := t.Ctx.Logger().WithField("lockID", lockID)


Restore the logs here. They can be helpful.

restored the previous logs

The transaction engine executes the given transaction across the cluster.The engine is designed to make use of etcd as the means of communication between peers. Please refer Design Document gluster#1003 cleanup leader: added a cleanup leader which will perform all cleaning operation. A leader is elected among the peers in the cluster to cleanup stale transactions. The leader periodically scans the pending transaction namespace for failed and stale transactions, and cleans them up if rollback is completed by all peers involved in the transaction. Signed-off-by: Oshank Kumar <okumar@redhat.com>

Signed-off-by: Oshank Kumar <okumar@redhat.com>

ghost assigned oshankkumar Oct 8, 2018

ghost added the in progress label Oct 8, 2018

oshankkumar force-pushed the newTxnFrmwrk branch from 4e2895e to 0a4df19 Compare October 8, 2018 13:35

kshlm requested review from prashanthpai and kshlm October 8, 2018 14:00

oshankkumar force-pushed the newTxnFrmwrk branch 2 times, most recently from 55bb33c to 89a0b73 Compare October 8, 2018 14:37

atinmu changed the title ~~transaction: Implemented Txn Engine~~ transaction: Resilient Txn Engine Oct 8, 2018

oshankkumar force-pushed the newTxnFrmwrk branch 3 times, most recently from 847ff5f to c537254 Compare October 10, 2018 12:37

oshankkumar force-pushed the newTxnFrmwrk branch 5 times, most recently from 8fe9508 to 1733ca1 Compare October 17, 2018 08:39

oshankkumar force-pushed the newTxnFrmwrk branch 11 times, most recently from 210a04f to 1055b69 Compare October 26, 2018 03:31

kshlm reviewed Oct 30, 2018

View reviewed changes

oshankkumar force-pushed the newTxnFrmwrk branch 7 times, most recently from a866695 to 526cb04 Compare November 5, 2018 13:15

kshlm reviewed Nov 6, 2018

View reviewed changes

oshankkumar force-pushed the newTxnFrmwrk branch from 526cb04 to 5181ab5 Compare November 7, 2018 07:00

atinmu mentioned this pull request Nov 10, 2018

doc: Add a design document for the transaction framework #1003

Merged

1 task

oshankkumar force-pushed the newTxnFrmwrk branch from 5181ab5 to 01a6f06 Compare November 14, 2018 13:02

kshlm reviewed Nov 20, 2018

View reviewed changes

oshankkumar force-pushed the newTxnFrmwrk branch from 0840cfa to cc26978 Compare November 21, 2018 10:39

Oshank Kumar and others added 5 commits December 11, 2018 15:44

transaction: used etcd Election to elect cleanup leader

a8a7372

Signed-off-by: Oshank Kumar <okumar@redhat.com>

transaction: removed duplicate code

81b0710

Signed-off-by: Oshank Kumar <okumar@redhat.com>

transaction: restore logs of txn lock

11f0033

Signed-off-by: Oshank Kumar <okumar@redhat.com>

Update transactions to use Sync

4357e26

kshlm force-pushed the newTxnFrmwrk branch from cc26978 to 4357e26 Compare December 11, 2018 12:07

ghost assigned kshlm Dec 11, 2018

kshlm added 2 commits December 11, 2018 18:48

Merge branch 'master' into newTxnFrmwrk

af426fe

Merge branch 'master' into newTxnFrmwrk

ce05148

kshlm approved these changes Dec 11, 2018

View reviewed changes

kshlm merged commit 5f88917 into gluster:master Dec 11, 2018

ghost removed the in progress label Dec 11, 2018

atinmu mentioned this pull request Dec 11, 2018

Transaction: Recovery from failure of undo #250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transaction: Resilient Txn Engine #1268

transaction: Resilient Txn Engine #1268

oshankkumar commented Oct 8, 2018

atinmu commented Oct 8, 2018

atinmu commented Oct 8, 2018

oshankkumar commented Oct 11, 2018 •

edited

Loading

kshlm left a comment

kshlm Oct 30, 2018

oshankkumar Oct 31, 2018

kshlm Oct 30, 2018

kshlm Oct 30, 2018

kshlm Oct 30, 2018

kshlm Oct 30, 2018

kshlm Oct 30, 2018

kshlm Oct 30, 2018

oshankkumar commented Nov 5, 2018

Madhu-1 commented Nov 5, 2018

kshlm left a comment

atinmu commented Nov 16, 2018

kshlm left a comment

kshlm Nov 20, 2018

oshankkumar Nov 21, 2018

transaction: Resilient Txn Engine #1268

transaction: Resilient Txn Engine #1268

Conversation

oshankkumar commented Oct 8, 2018

atinmu commented Oct 8, 2018

atinmu commented Oct 8, 2018

oshankkumar commented Oct 11, 2018 • edited Loading

Task List

kshlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshankkumar commented Nov 5, 2018

Madhu-1 commented Nov 5, 2018

kshlm left a comment

Choose a reason for hiding this comment

atinmu commented Nov 16, 2018

kshlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshankkumar commented Oct 11, 2018 •

edited

Loading