[LEDGER] move couchdb util package to statecouchdb package #926

cendhu · 2020-03-27T18:58:47Z

Type of change

Improvement (improvement to code)

Description

We have a CouchDB util package that exposes API to read from and write to CouchDB. This package is used only by the statecouchdb and the tests package (i.e., ledger level integration test).

In general, a util package is justified only when an API needs to be used by so many other packages. For e.g., if we have 10 packages and each of them need a common API, it is not good to duplicate the API code. Hence, it is good to create a separate util package. One such example can be found in the volume folder in the Kubernetes repo – It can be seen that Kubernetes supports 26 different volume types and hence a util package to hold common APIs is justified. However, in our case, it is not justified to have a separate util package to facilitate CouchDB access.

Hence, we move all files in the CouchDB util package to the statecouchdb package itself. Note that there is a lot of opportunities to refactor the CouchDB util package but we would do it in a separate JIRA.

Additional details

Before starting the ledger checkpointing code, we would like to refactor the ledger package. If we continue to add new features with the current package structure, after a few years, it will be unmaintainable. We plan to have nearly 10 such PRs. Note that each PR may modify many files and we cannot do much about it. We might be okay to get some review burden than having an unmaintainable code in the longer term.

Related issues

FAB-17667

core/ledger/ledger_interface.go

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go

cendhu · 2020-03-30T03:39:25Z

gossip UT flake

cendhu · 2020-03-30T03:40:02Z

/ci-run

github-actions · 2020-03-30T03:40:41Z

AZP build triggered!

core/ledger/kvledger/txmgmt/statedb/statecouchdb/couchdb_test.go

core/ledger/kvledger/tests/util.go

core/ledger/kvledger/txmgmt/privacyenabledstate/test_exports.go

manish-sethi

A few comments on the changes in your last change-set

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go

core/ledger/kvledger/txmgmt/statedb/statecouchdb/couchdb_test.go

core/ledger/util/couchdb/metrics_test.go

core/ledger/util/couchdb/couchdbutil_test.go

cendhu · 2020-04-02T01:44:53Z

Yes, Manish. I catched all your points already and that's why I made it WIP. Will be done by today.

manish-sethi · 2020-04-02T02:33:43Z

Yes, Manish. I catched all your points already and that's why I made it WIP. Will be done by today.

Yes, My fault. I noticed WIP tag, after submitting the comments

cendhu · 2020-04-02T09:49:03Z

@ryjones I have a question as you asked me to squash my commits to one.

Do we have this squash and merge feature? https://blog.pairworking.com/why-you-should-care-about-squash-and-merge-in-git-675856bf66b0

If it is so, I will push multiple commits per PRs as it would enable the reviewer to easily look at changes made between two reviews. Otherwise, the reviewer's job would be painful to go over all files. During the merge, they can squash and merge.

@manish-sethi also feels that it is hard to review without seeing the diff between the base commit and new commits which made changes to code as per the suggestions.

manish-sethi · 2020-04-02T14:30:02Z

@ryjones I have a question as you asked me to squash my commits to one.

Do we have this squash and merge feature? https://blog.pairworking.com/why-you-should-care-about-squash-and-merge-in-git-675856bf66b0

If it is so, I will push multiple commits per PRs as it would enable the reviewer to easily look at changes made between two reviews. Otherwise, the reviewer's job would be painful to go over all files. During the merge, they can squash and merge.

@manish-sethi also feels that it is hard to review without seeing the diff between the base commit and new commits which made changes to code as per the suggestions.

@cendhu - We can still see the diffs even if you push as a single commit. The problem is when you rebase in between

cendhu · 2020-04-02T15:50:50Z

Conflicts are resolved. @manish-sethi let's get this in. This PR is too boring now.

manish-sethi

Overall LGTM. Just one comment on what you changed in the last patch

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go

cendhu

I have addressed the review comments. I have pushed multiple commits so that the changes can be seen easily.

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go

cendhu · 2020-04-14T15:49:01Z

rebased and fixed conflicts.

manish-sethi

LGTM, mostly cleanup related comments.

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test.go

core/ledger/kvledger/txmgmt/statedb/statecouchdb/couchdb_test.go

core/ledger/kvledger/txmgmt/statedb/statecouchdb/couchdb_test_export.go

manish-sethi · 2020-04-15T14:38:31Z

@cendhu - LGTM now. You can squash and make it a single commit and we can merge now. Only one pending comment is there. If you want to address that while making it as a single commit, that would be good. But I am fine merging otherwise as well.

We have a CouchDB util package that exposes API to read from and write to CouchDB. This package is used only by the statecouchdb and the tests package (i.e., ledger level integration test). In general, a util package is justified only when an API needs to be used by so many other packages. For e.g., if we have 10 packages and each of them need a common API, it is not good to duplicate the API code and instead create a separate util package. Hence, we move all files in the CouchDB util package to the statecouchdb package itself. Note that there is a lot of opportunities to refactor couchdb package but we will do that in a separate PR. Given that we have moved couchdb util to the statecouchdb package, we can have only one TestMain. It would be hacky to reuse existing testVDBEnv for testing couchDB too. Hence, we introduce another testEnv called testCouchDBEnv to test couchDB utils. Signed-off-by: senthil <cendhu@gmail.com>

cendhu · 2020-04-15T15:21:42Z

Squashed all 3 commits to a single commit.

cendhu requested a review from a team as a code owner March 27, 2020 18:58

cendhu changed the title ~~[for v2.2.0] ledger refactoring (1) -- move couchdb util package to statecouchdb package~~ [for v2.2.0] move couchdb util package to statecouchdb package Mar 27, 2020

cendhu force-pushed the mv-couchdb-util branch from 7586deb to bad633f Compare March 27, 2020 19:12

cendhu requested a review from a team as a code owner March 27, 2020 19:12

mastersingh24 reviewed Mar 28, 2020

View reviewed changes

core/ledger/ledger_interface.go Outdated Show resolved Hide resolved

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go Outdated Show resolved Hide resolved

cendhu changed the title ~~[for v2.2.0] move couchdb util package to statecouchdb package~~ [for v2.2.0] move couchdb util package to statecouchdb package [1] Mar 30, 2020

cendhu force-pushed the mv-couchdb-util branch from 0e781a4 to 0fdf1c9 Compare March 30, 2020 13:12

cendhu changed the title ~~[for v2.2.0] move couchdb util package to statecouchdb package [1]~~ move couchdb util package to statecouchdb package [merge-order-1] Mar 31, 2020

manish-sethi requested changes Apr 1, 2020

View reviewed changes

core/ledger/kvledger/txmgmt/statedb/statecouchdb/couchdb_test.go Show resolved Hide resolved

core/ledger/kvledger/tests/util.go Outdated Show resolved Hide resolved

core/ledger/kvledger/txmgmt/privacyenabledstate/test_exports.go Outdated Show resolved Hide resolved

cendhu force-pushed the mv-couchdb-util branch from 0fdf1c9 to b4de8d5 Compare April 1, 2020 09:43

cendhu changed the title ~~move couchdb util package to statecouchdb package [merge-order-1]~~ [WIP] move couchdb util package to statecouchdb package [merge-order-1] Apr 1, 2020

manish-sethi requested changes Apr 1, 2020

View reviewed changes

cendhu force-pushed the mv-couchdb-util branch 3 times, most recently from 8dc4456 to 17b78a7 Compare April 2, 2020 09:41

cendhu changed the title ~~[WIP] move couchdb util package to statecouchdb package [merge-order-1]~~ move couchdb util package to statecouchdb package Apr 2, 2020

cendhu force-pushed the mv-couchdb-util branch from 17b78a7 to 522c568 Compare April 2, 2020 15:50

cendhu force-pushed the mv-couchdb-util branch from 522c568 to 6bac872 Compare April 2, 2020 16:48

manish-sethi requested changes Apr 2, 2020

View reviewed changes

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go Outdated Show resolved Hide resolved

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go Outdated Show resolved Hide resolved

cendhu force-pushed the mv-couchdb-util branch from 6bac872 to 36282c5 Compare April 9, 2020 12:05

cendhu commented Apr 9, 2020

View reviewed changes

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go Outdated Show resolved Hide resolved

core/ledger/kvledger/txmgmt/statedb/statecouchdb/statecouchdb_test_export.go Outdated Show resolved Hide resolved

cendhu force-pushed the mv-couchdb-util branch from 130cd9a to 028c590 Compare April 9, 2020 15:08

cendhu changed the title ~~move couchdb util package to statecouchdb package~~ [LEDGER] move couchdb util package to statecouchdb package Apr 9, 2020

cendhu force-pushed the mv-couchdb-util branch 5 times, most recently from 462a0d0 to 78111cb Compare April 14, 2020 15:47

manish-sethi requested changes Apr 14, 2020

View reviewed changes

cendhu force-pushed the mv-couchdb-util branch from 33bb1fc to d6acd97 Compare April 15, 2020 08:46

cendhu force-pushed the mv-couchdb-util branch from d6acd97 to 8c6f7e9 Compare April 15, 2020 15:20

manish-sethi approved these changes Apr 15, 2020

View reviewed changes

manish-sethi merged commit c41f9eb into hyperledger:master Apr 15, 2020

cendhu deleted the mv-couchdb-util branch April 18, 2020 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LEDGER] move couchdb util package to statecouchdb package #926

[LEDGER] move couchdb util package to statecouchdb package #926

cendhu commented Mar 27, 2020

cendhu commented Mar 30, 2020

cendhu commented Mar 30, 2020

github-actions bot commented Mar 30, 2020

manish-sethi left a comment

cendhu commented Apr 2, 2020

manish-sethi commented Apr 2, 2020

cendhu commented Apr 2, 2020

manish-sethi commented Apr 2, 2020

cendhu commented Apr 2, 2020

manish-sethi left a comment

cendhu left a comment

cendhu commented Apr 14, 2020

manish-sethi left a comment

manish-sethi commented Apr 15, 2020

cendhu commented Apr 15, 2020

[LEDGER] move couchdb util package to statecouchdb package #926

[LEDGER] move couchdb util package to statecouchdb package #926

Conversation

cendhu commented Mar 27, 2020

Type of change

Description

Additional details

Related issues

cendhu commented Mar 30, 2020

cendhu commented Mar 30, 2020

github-actions bot commented Mar 30, 2020

manish-sethi left a comment

Choose a reason for hiding this comment

cendhu commented Apr 2, 2020

manish-sethi commented Apr 2, 2020

cendhu commented Apr 2, 2020

manish-sethi commented Apr 2, 2020

cendhu commented Apr 2, 2020

manish-sethi left a comment

Choose a reason for hiding this comment

cendhu left a comment

Choose a reason for hiding this comment

cendhu commented Apr 14, 2020

manish-sethi left a comment

Choose a reason for hiding this comment

manish-sethi commented Apr 15, 2020

cendhu commented Apr 15, 2020