Rename transactionv2 to transaction #1424
Conversation
ghost
assigned 
@kshlm CI failure |
glusterd2/transaction/context.go
Outdated
| @@ -139,7 +137,7 @@ func (c *Tctx) Commit() error { | |||
| return err | |||
| } | |||
|
|
|||
| expTxn.Add("txn_ctx_store_commit", 1) | |||
| //expTxn.Add("txn_ctx_store_commit", 1) | |||
glusterd2/transaction/context.go
Outdated
| @@ -208,7 +206,7 @@ func (c *Tctx) Delete(key string) error { | |||
| "failed to delete key") | |||
| return err | |||
| } | |||
| expTxn.Add("txn_ctx_store_delete", 1) | |||
| //expTxn.Add("txn_ctx_store_delete", 1) | |||
glusterd2/transaction/transaction.go
Outdated
| Nodes []uuid.UUID | ||
| OrigCtx context.Context | ||
| Nodes []uuid.UUID `json:"nodes"` | ||
| StorePrefix string `json:"store_prefix"` |
There was a problem hiding this comment.
json value should not contain _, use - store_prefix to store-prefix
| StartTime time.Time `json:"start_time"` | ||
|
|
||
| success chan struct{} | ||
| error chan error |
There was a problem hiding this comment.
error is a package name, can we rename this variable name to something else?
There was a problem hiding this comment.
error here is a member of the Txn struct, and doesn't affect the error name in the global namespace.
glusterd2/transaction/transaction.go
Outdated
| t.Ctx.Logger().Debug("new transaction created") | ||
| return t | ||
| } | ||
|
|
||
| // NewTxnWithLocks returns an empty Txn with locks obtained on given lockIDs | ||
| func NewTxnWithLocks(ctx context.Context, lockIDs ...string) (*Txn, error) { | ||
| t := NewTxn(ctx) | ||
| t.Locks = lockIDs | ||
| return t, nil |
There was a problem hiding this comment.
can we discard returning error from this function?
There was a problem hiding this comment.
@Madhu-1 then we have to make changes in all files where this function has been called. Maybe we can take this up later.
| } | ||
|
|
||
| expTxn.Add("initiated_txn_in_progress", -1) | ||
| } | ||
|
|
||
| func (t *Txn) checkAlive() error { |
There was a problem hiding this comment.
can you please move nodes union logic from checkAlive() to txn.Do()
if len(t.Nodes) == 0 {
for _, s := range t.Steps {
t.Nodes = append(t.Nodes, s.Nodes...)
}
}
t.Nodes = nodesUnion(t.Nodes)
In case if some user set txn.DontCheckAlive as true, and don't provide the Nodes information in Txn Object then we need nodes union logic before starting the transaction to fill the Nodes info in Txn object.
There was a problem hiding this comment.
due to this only some e2e tests are failing
|
3 tests keep failing.
TestBitrot/Replica-volume fails because the RPC done to the scrub daemon to fetch status, returns TestBitrot/Dist-volume fails as the scrub daemon isn't running. GD2 isn't able to connect to the daemon to send the status request. These two tests switch the failures depending on the order they run. The first test always fails because of empty dict, and the second fails because of failure to connect. The Snapshot test fetches a list of snapshots, and the list returned is empty. This causes a out-of-index panic later when the first item in the list is accessed. This happens even though the previous tests are successful and snapshots should be present. What is surprising to me is that all of these pass without this PR. This PR essentially doesn't change anything about the execution of transaction steps. The steps should still continue to execute as before and should work. I need to look into this more to get to the cause. |
|
retest this please |
... and remove all usage of glusterd2/oldtransaction from glusterd2/transaction.
kshlm
force-pushed
the
remove-old-txn
branch
from
e53e656 to
2eccd21
Compare
This commit does 3 things,
the transactionv2 package.