[LEDGER] reduce #arguments in a few kvledger methods

[cendhu]

Type of change

• Improvement (improvement to code)

Description

As the number of arguments passed to newKVLedger(), initTxMgr(), and NewLockBasedTxMgr() is high, we reduce it by passing the initializer struct itself.

Additional details

In subsequent PRs, we would think of moving the BookeepingProvider and DB to the txmgr pkg and instead create a TxMgrProvider in the kvLedger to reduce the number of arguments further by 1. Moreover, it is natural to initiate structs that are directly used by kvLedger in the kvledger pkg. For now, the BookeepingProvider and DB are not directly used by the kvLedger.

Related issues

FAB-17683

[manish-sethi]

@cendhu - I don't think that this change is in the right direction. The dependencies were being passed explicitly, as is typically desired.

[lindluni]

@cendhu - I don't think that this change is in the right direction. The dependencies were being passed explicitly, as is typically desired.

I don't have an extensive background on what is right or wrong here, but I tend to support this change. The simplification greatly reduced the complexity of following the code. The object being passed around (and it remains in the kvledger package, and in the same file) the same package until it is ultimately passed off to another package (the txmgr) where the necessary parameters are extracted from the object before being passed out of the package.

If I was tracing this code I have to track the same 5 parameters through multiple function calls, making it difficult to keep track of mentally. With Senthil's change I can simply follow the object until it's ultimately extracted within the current package and passed off for consumption to a single function. The context is much simpler.

It's also worth noting, that if you were to turn on the code smell feature of many of the most popular Golang linting tools, a function taking in this many number of arguments would NOT pass the smell tests as being unusually complicated and not being mentally digestible.

As is typically desired doesn't always mean mandatory. The reduction in complexity IMO is worth breaking from what might be typically desired.

[lindluni]

you're already passing in the lgrProvider you can pull the lgrProvider.stateListeners off the object in the callee instead of passing it down explicitly.

[manish-sethi]

@cendhu - I don't think that this change is in the right direction. The dependencies were being passed explicitly, as is typically desired.

I don't have an extensive background on what is right or wrong here, but I tend to support this change. The simplification greatly reduced the complexity of following the code. The object being passed around (and it remains in the kvledger package, and in the same file) the same package until it is ultimately passed off to another package (the txmgr) where the necessary parameters are extracted from the object before being passed out of the package.

If I was tracing this code I have to track the same 5 parameters through multiple function calls, making it difficult to keep track of mentally. With Senthil's change I can simply follow the object until it's ultimately extracted within the current package and passed off for consumption to a single function. The context is much simpler.

It's also worth noting, that if you were to turn on the code smell feature of many of the most popular Golang linting tools, a function taking in this many number of arguments would NOT pass the smell tests as being unusually complicated and not being mentally digestible.

As is typically desired doesn't always mean mandatory. The reduction in complexity IMO is worth breaking from what might be typically desired.

I am not against clubbing the parameters to reduce the number of parameters. My objection is to the way it is achieved here. i.e., pass down the provider instance much deeper in the code where we are supposed to have the code specific to one ledger instance. This is overusing the provider, which is not merely a holder of objects but has an associated behavior as well.
For instance, the same problem was there when passing the dependencies from peer to ledger and they were clubbed in this struct -

fabric/core/ledger/ledgermgmt/ledger_mgmt.go

Line 45 in 7778c49

type Initializer struct {

[cendhu]

@manish-sethi As @btl5037 mentioned, having too many arguments make the code readability a bit hard. I have faced it multiple times when adding/making changes to kvledger package. Let me share the thought process I had before submitting this PR.

1. given that newKVLedger() uses many variables present in the Provider, I thought of moving newKVLedger() method under Provider struct itself and renaming it to initKVLedger() but that might break the current file organization and wasn't sure whether you would approve it. In the earlier days, kv_ledger_provider.go and kv_ledger.go confused me a lot and now I know about them. Anyway, I didn't want to do a bigger refactoring in this PR.
2. when we call newKVLedger(), we do pass most of the arguments needed to construct the kvLedger struct but txmgr. Hence, I was thinking of directly creating kvLedger struct from the Provider and calling kvLedger.init() but again I wasn't sure whether newXXX only should create the sturct and hence, I dropped that option too. Moreover, we pass all needed items from Provider except txmgr which did not look correct to me. As can be seen in the PR description, I am planning to introduce something called txmgr provider.
3. finally I was thinking of introducing a struct called ledgerConfig and txmgrConfig but I thought this might collide with the ledgerConfig passed from start.go.

I didn't think of the initializer struct. Are you suggesting to introduce something like kvLedgerInitializer struct and pass it to newKVLedger()? Similarly, we can introduce txmgrInitializer to reduce the number of arguments in initTxmgr()

[manish-sethi]

1. given that newKVLedger() uses many variables present in the Provider, I thought of moving newKVLedger() method under Provider struct itself and renaming it to initKVLedger()

I am fine with this approach, if you want to go that way.

Moreover, we pass all needed items from Provider except txmgr which did not look correct to me. As can be seen in the PR description, I am planning to introduce something called txmgr provider.

There is a difference between the other items that are passed in and the txmgr. The other items are either passed-in dependencies from other packages (e.g., DeployedChaincodeInfoProvider) or they are shared stores (e.g., leveldbs), whereas, the txmgr is just an internal struct which is constructed one for each ledger - and hence constructed internally.

I didn't think of the initializer struct. Are you suggesting to introduce something like kvLedgerInitializer struct and pass it to newKVLedger()? Similarly, we can introduce txmgrInitializer to reduce the number of arguments in initTxmgr()

Yes, that's what I meant.

So, either (or mix of both) of provider.newKVLedger() and txmgrInitializer works for me.

Finally, IMO, the bigger problem with this function is that some of the dependencies of initialization ordering needs a revisit... so, various init's can be removed

[mastersingh24]

@manish-sethi is on to something here. I'd say that this is worthy of a bigger refactor (although I do see the value in reducing the number of parameters where possible).

We really need to define the hierarchy of structs here and try to avoid side effects as best as possible. For example, does a provider have multiple ledgers or do ledgers have a provider (where multiple ledgers can share a provider)

[cendhu]

@mastersingh24 Yes, Manish was planning to refactor these dependencies ( some are ledger sub-packages and some are not) as part of validation refactoring epic (if I remember correctly). For now, we have decided to defer a few bigger refactoring work as we need to move to ledger checkpointing work. We would still do small refactoring while we code for the ledger checkpointing and defer the bigger refactoring which involves the following packages:

1. mvcc validation pkg.
2. kvledger pkg
3. merge kvledger pkg with ledger pkg to reduce folder depth.
4. merge lockbasedtxmgr pkg with the txmgr pkg.
5. merge privacyenabledstate pkg with the lockbasedtxmgr pkg

and a few more.

[mastersingh24]

@mastersingh24 Yes, Manish was planning to refactor these dependencies ( some are ledger sub-packages and some are not) as part of validation refactoring epic (if I remember correctly). For now, we have decided to defer a few bigger refactoring work as we need to move to ledger checkpointing work. We would still do small refactoring while we code for the ledger checkpointing and defer the bigger refactoring which involves the following packages:

1. mvcc validation pkg.
2. kvledger pkg
3. merge kvledger pkg with ledger pkg to reduce folder depth.
4. merge lockbasedtxmgr pkg with the txmgr pkg.
5. merge privacyenabledstate pkg with the lockbasedtxmgr pkg

and a few more.

Got it - makes sense

[manish-sethi]

In isolation, without checking initializer for nil, this check seems a half job. Also, we check here only for Hasher and not for other parameters, seem odd. Though, the difference is that the hasher is an external ledger dependency that is passed from the peer. So, I guess that to make it clear, we should move this way up (directly when the ledger is initialized from outside). Not introduced in this PR, so can be done separately.

[cendhu]

I agree. At least we can move this check to kvledger package for now.