[LEDGER] merge queryHelper with the queryExecutor #1097
Conversation
There was a problem hiding this comment.
LGTM except one comment about making the structs exported type.
On this side, I would also like to suggest that the reviews would be easier if you can make changes in stages. For instance, in this PR, you moved functions around at the same time which redendered whole diff. Changes in this PR could be split into three
- Removal of queryexector + renaming the helper
- Merging of unexported and exported functions, where applicable
- Moving functions around
- introduce flag instead of checking rwsetBuilder != nil
| type lockBasedTxSimulator struct { | ||
| lockBasedQueryExecutor | ||
| // TxSimulator is a transaction simulator used in `LockBasedTxMgr` | ||
| type TxSimulator struct { |
There was a problem hiding this comment.
Not sure why you made this exported. This should not be constructed outside this package. Same comment about QueryExecutor
There was a problem hiding this comment.
Unexported struct implementing an exported function looks weird. Some discussion on this happened in #1094 Hence, I exported the struct.
Btw, the initial goal was to first merge #1094, second merge #1095, and finally make the following change in this PR as mentioned in the PR description. NewTxSimulation() and NewQueryExecutor() to return the struct instead of an interface as it is a widely suggested way when there is a single implementation for an interface. Now, the above-mentioned change will be done once the post-order tx execution is moved from validator to the lockbasedtxmgr and the TxMgr interface is removed.
Sure. Going forward, I will push the stacked commits. It is helpful for the review and also to not waste time on fixing conflicts. |
In lockbasedtxmgr, we have a lockBasedQueryExecutor and a lockBasedTxSimulator struct. The lockBasedTxSimulator implements mainly SetXXX() APIs while lockBasedQueryExecutor implements GetXXX() APIs. As the transaction simulation also involves query, the lockBasedTxSimulator encapsulate lockBasedQueryExecutor. Only in the lockBasedTxSimulator, we collect readSet while using GetXXX() and writeSet while using SetXXX(). However, when we use lockBasedQueryExecutor alone, we don't collect readSet. Currently, we have an unnecessary indirection with QueryHelper and it makes code readability a bit hard. Hence, we merge the QueryHelper with the lockBasedQueryExecutor itself and do some renaming of files/structs too. Signed-off-by: senthil <cendhu@gmail.com>
Signed-off-by: senthil <cendhu@gmail.com>
3d55670 to
802e617
Compare
|
Fixed conflicts and review comments. |
Type of change
Description
In the lockbasedtxmgr package, we have a
lockBasedQueryExecutorand alockBasedTxSimulatorstruct. The lockBasedTxSimulator implements mainlySetXXX()APIs while the lockBasedQueryExecutor implementsGetXXX()APIs. As the transaction simulation also involves query, the lockBasedTxSimulator encapsulate lockBasedQueryExecutor.Only in the lockBasedTxSimulator, we collect readSet while using
GetXXX()and writeSet while usingSetXXX(). However, when we use lockBasedQueryExecutor alone, we don't collect readSet.Currently, we have an unnecessary indirection with
QueryHelperand it makes code readability a bit hard. Hence, we merge theQueryHelperwith thelockBasedQueryExecutoritself.Further, due to stutters, we rename
lockBasedTxSimulator=>txSimulatorlockBasedQueryExecutor=>queryExecutorlockbased_query_executor.go=>query_executor.golockbased_tx_simulator.go=>tx_simulator.goAdditional details
Note that
NewQueryExecutor()andNewTxSimulator()return interface rather than a struct. It will be fixed when we remove theTxMgrinterface.Related issues
FAB-17760