Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FAB-17795] Build channel metadata retroactively if not present at peer start #1369

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

wenjianqiao
Copy link
Contributor

@wenjianqiao wenjianqiao commented Jun 3, 2020

Signed-off-by: Wenjian Qiao wenjianq@gmail.com

Type of change

  • New feature

Description

Channel metadata has been added to statecouchdb to store the mapping of db namespaces
to db names. However, the channel metadata is not available in v2.0/v2.1 peers. This PR
adds support to retroactively build channel metadata at peer start if such metadata
is not present in statecouchdb.

When a state CouchDB is opened (GetDBHandle), it retroactively builds channel metadata if
the metadata is not present. A NamespaceProvider is implemented to build possible namespaces for the channel. The possible namespaces will be verified by the existing databases and only namespaces matching existing databases will be added to channel metadata.

Additional details

The channel metadata will be used to remove per-channel databases and support checkpoint snapshots (e.g., create snapshot, clean up after bootstrap failure)

Related issues

https://jira.hyperledger.org/browse/FAB-17795
https://jira.hyperledger.org/browse/FAB-17787

@wenjianqiao wenjianqiao requested a review from a team as a code owner June 3, 2020 20:11
Copy link
Contributor

@cendhu cendhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Wenjian. I have suggestions related to the overall approach.

@wenjianqiao wenjianqiao force-pushed the fab17795 branch 2 times, most recently from e3c09f6 to e460c59 Compare June 6, 2020 01:25
@wenjianqiao
Copy link
Contributor Author

wenjianqiao commented Jun 6, 2020

@cendhu Thank you for your comments. As discussed in our scrum, I have refactored the code and moved channelMetadata rebuild to newVersionedDB. I have addressed your comments unless the code that has been removed due to refactoring.
The SimpleQueryExecutor and results iterator wrapper structs are remained the same because it requires refactoring.

Copy link
Contributor

@cendhu cendhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Wenjian, for the changes. I have a few more suggestions. Once they are addressed, I will look at the test.

Copy link
Contributor

@manish-sethi manish-sethi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks good to me. However, I have a couple comments, mostly related to reorganizing the code a bit for a better separation of concerns.

Channel metadata has been added to statecouchdb to store the mapping of db namespaces
to db names. However, the channel metadata is not available in v2.0/v2.1 peers. This PR
adds support to retroactively build channel metadata at peer start if such metadata
is not present in statecouchdb.

When a state CouchDB is opened (GetDBHandle), it retoactively builds channel metadata if
the metadata is not present. A NamespaceProvider is implemented to build possible namespaces
for the channel. The possible namespaces will be verified by the existing databases and
only namespaces matching existing databases will be added to channel metadata.

Signed-off-by: Wenjian Qiao <wenjianq@gmail.com>
Copy link
Contributor

@manish-sethi manish-sethi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, if you agree and want to address in a separate PR.

Comment on lines +433 to +434
// channelInfoProvider interface enables the privateenabledstate package to retrieve all the config blocks
// and namespaces and collections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to update the comments?
I though that at this level, perhaps you would name this a bit more restrictive like nsCollNames or nsCollNamesProvider instead of a generic name channelInfoProvider

Comment on lines +594 to +597
// define this interface to break circular dependency
type channelInfoProviderWrapper interface {
channelInfoProvider
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure whether this wrapper is needed, as the actual interface itself is unexported.

// and namespaces and collections.
type channelInfoProvider interface {
// NamespacesAndCollections returns namespaces and collections for the channel.
NamespacesAndCollections(vdb statedb.VersionedDB) (map[string][]string, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earlier division was better where the query executor was still in this package and this function could the queryexector but it's fine for now.

}

// test an existing DB with channelMetadata, namespace provider should not be called
require.NoError(t, vdb.initChannelMetadata(false, fakeNsProvider))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test with "true" flag could also be added.

@manish-sethi manish-sethi merged commit 6c27f01 into hyperledger:master Jun 9, 2020
xhens added a commit to xhens/fabric that referenced this pull request Jun 12, 2020
* Add FullScanIterator to export channel's data present in the CouchDB (hyperledger#1348)

* implement FullScanIterator for couchdb

This commit implements statedb.FullScanIterator interface
in the statecouchdb pkg for getting access to an iterator that
can be used for scanning the entire state, including private
data hashes for a particular channel

FAB-17901

Signed-off-by: senthil <cendhu@gmail.com>

* address review comments

Signed-off-by: senthil <cendhu@gmail.com>

* address review comments

Signed-off-by: senthil <cendhu@gmail.com>

* FAB-17912 Ch.Part.API: reject joins (hyperledger#1375)

Orderer reject joins:
1. when system channel exists, or
2. if channel exists

Signed-off-by: Yoav Tock <tock@il.ibm.com>
Change-Id: Ia68bcc91fa7d9363f2320ad69439eedb7a7aeab2

* FAB-17967 Updates for new language contributors

Makes it easier for new contributors to the documentation to get started,
including international language translators.

Signed-off-by: Anthony O'Dowd <a_o-dowd@uk.ibm.com>

* Update fabric-config dep to v0.0.4 and associated int. test (hyperledger#1378)

FAB-17956 #done

Signed-off-by: Will Lahti <wtlahti@us.ibm.com>

* Update private data tutorial for contract api

Signed-off-by: NIKHIL E GUPTA <negupta@us.ibm.com>

* [FAB-17795] Build channel metadata retroactively if not present (hyperledger#1369)

Channel metadata has been added to statecouchdb to store the mapping of db namespaces
to db names. However, the channel metadata is not available in v2.0/v2.1 peers. This PR
adds support to retroactively build channel metadata at peer start if such metadata
is not present in statecouchdb.

When a state CouchDB is opened (GetDBHandle), it retoactively builds channel metadata if
the metadata is not present. A NamespaceProvider is implemented to build possible namespaces
for the channel. The possible namespaces will be verified by the existing databases and
only namespaces matching existing databases will be added to channel metadata.

Signed-off-by: Wenjian Qiao <wenjianq@gmail.com>

* Regenerate proto to sync the go code

The source file name in the generated code is somehow different
and hence it shows the difference when running make protos.

Signed-off-by: manish <manish.sethi@gmail.com>

* Fix the constructor message in complex queries

Signed-off-by: zhuzeyu <zhuzeyu0409@gmail.com>

* add rlock to IsEmpty() in leveldb wrapper (hyperledger#1388)

This commit adds rlock to IsEmpty() to synchronize
between Close() and IsEmpty().

Signed-off-by: senthil <cendhu@gmail.com>

* Bump Go and Alpine Version

Bump Go to 1.14.4. This is also the first Go release
to switch to alpine 3.12 with 1.14.3 being the last
to support 3.11. Performing this update now will allow
us to cross the alpine minor version barrier prior
to our LTS release of 2.2 to give us time to test
the stability of the release.

Signed-off-by: Brett Logan <brett.t.logan@ibm.com>

* Bump Go Version in Vagrant

Signed-off-by: Brett Logan <brett.t.logan@ibm.com>

* FAB-17951 fetch correct node id for validation (hyperledger#1367)

* FAB-17951 fetch correct node id for validation

Today, when a config update that modifies consenter set (add/remove
node), there's validator performing basics checks, including dangerous
configs that may result in quorum loss, e.g. removing active node from
a network with 2/3 alive nodes.

However, this validator always assumes node ID in consenter set to start
from 1. This is benign in most cases, as nodes are normally online, and
actual node ID doesn't really matter from validation pov. Although, in
certain special cases, this is problematic:
- we have [2, 3, 4] in consenter set
- [2, 3] are alive, and [4] is inactive and subject to remove
- if validator assume node starts from 1, then it would incorrectly conclude
  that [3] out of [1, 2, 3] is to be removed, while [2, 3] are the 2/3 alive
  nodes. Therefore, it would reject such request
- instead, we need to take actual node IDs into account

This commit fixes the problem and adds some UTs. IT will be added in a seperate
commit.

Signed-off-by: Jay Guo <guojiannan1101@gmail.com>

* Amend IT to assert active node shrink

An integration test is amended to assert that number of
active nodes shrinks when a node is offline.

Signed-off-by: Jay Guo <guojiannan1101@gmail.com>

* Update Pipfile to support localization

Signed-off-by: pama-ibm <pama@ibm.com>

* add iter.Seek() in leveldb wrapper (hyperledger#1390)

This commits add iter.Seek() API in the leveldb wrapper.

Signed-off-by: senthil <cendhu@gmail.com>

* Fixed a typo (that that)

Signed-off-by: Vincent Déniel <denielvincent@gmail.com>

Co-authored-by: Senthil Nathan N <cendhu@users.noreply.github.com>
Co-authored-by: Yoav Tock <tock@il.ibm.com>
Co-authored-by: Anthony O'Dowd <a_o-dowd@uk.ibm.com>
Co-authored-by: Will Lahti <wtlahti@us.ibm.com>
Co-authored-by: NIKHIL E GUPTA <negupta@us.ibm.com>
Co-authored-by: Wenjian Qiao <wenjianq@gmail.com>
Co-authored-by: manish <manish.sethi@gmail.com>
Co-authored-by: zhuzeyu <zhuzeyu0409@gmail.com>
Co-authored-by: Brett Logan <brett.t.logan@ibm.com>
Co-authored-by: Jay Guo <guojiannan1101@gmail.com>
Co-authored-by: pama-ibm <pama@ibm.com>
Co-authored-by: Vincent DENIEL <vincentdnl@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants