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-11732] Add support for private data pagination in query results #1289

Closed
wants to merge 2 commits into from

Conversation

chanioxaris
Copy link

Signed-off-by: Haris Chaniotakis chanioxaris@gmail.com

Type of change

  • New feature
  • Documentation update

Description

Add support to query private data with pagination. Minimal changes needed, as there was already support for querying state data with pagination.

Stub interface affected, adding a new GetPrivateDataQueryResultWithPagination() function and the implementation.
Also updated the documentation regarding private data available functions.

Additional details

Related issues

https://jira.hyperledger.org/browse/FAB-11732

Signed-off-by: Haris Chaniotakis <chanioxaris@gmail.com>
…ction

Signed-off-by: Haris Chaniotakis <chanioxaris@gmail.com>
@chanioxaris chanioxaris requested review from a team as code owners May 19, 2020 15:22
@chanioxaris chanioxaris reopened this May 19, 2020
@cendhu
Copy link
Contributor

cendhu commented May 19, 2020

Thank you, @chanioxaris, for the contributions. New features would be added only to the master branch. This PR is for release-1.4. We only port bug fixes to release-1.4. Further, go chaincode shim has been moved a new repo: https://github.com/hyperledger/fabric-chaincode-go

There are some challenges with respect to range query on the private data: in a rare scenario, some private data could be missing in the statedb due to network reachability issues or network partition. The pvtdata reconciler would fetch those missing data periodically. We need to have some special handling of missing private data in the range query. I understand that this JIRA is an existing one and old but still it is good to initiate a discussion on #fabric-ledger (rocket chat) before proceeding with the code.

If paginated queries are used to export data out of peer, I would prefer introducing separate cli command by exploiting ledger checkpoint feature to dump public data and private data (but for private data, we need to add extra code as it is not needed in the checkpoint epic).

Comment on lines +331 to +333
GetPrivateDataQueryResultWithPagination(collection, query string, pageSize int32,
bookmark string) (StateQueryIteratorInterface, *pb.QueryResponseMetadata, error)

Copy link
Contributor

Choose a reason for hiding this comment

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

If this gets ported to fabric-chaincode-go, it should move to a separate interface to prevent breaking existing implementations. This is a side effect of a questionable decision of long ago to define the interface where implemented instead of using concrete types.

@chanioxaris
Copy link
Author

Hello @cendhu and thanks for your response.

Regarding the release and considering that v1.4 is a LTS version, i though that you can add a new expansion feature to existing shim interface. Adding this only to v2 will force production services that run on stable v1.4, such as ours, to upgrade to v2 to use it. Of course can be added on v2 as well, in a future release.

As you mentioned correctly i used an existing and old JIRA ticket to provide this addition. Today i created a new one FAB-17919 and linked any other relevant issues to share your thoughts, concerns and start the discussion from a fresh base.

@cendhu
Copy link
Contributor

cendhu commented May 20, 2020

Regarding the release and considering that v1.4 is a LTS version, i though that you can add a new expansion feature to existing shim interface. Adding this only to v2 will force production services that run on stable v1.4, such as ours, to upgrade to v2 to use it. Of course can be added on v2 as well, in a future release.

LTS release strategy can be found here -- https://hyperledger.github.io/fabric-rfcs/text/0000-lts-release-strategy.html As mentioned in this page, new features would be added in the master only

  1. New features and other changes can quickly be applied to the master branch, and distributed to the user community for trial, without impacting production deployments.
  2. Community feedback on new features can be solicited and acted upon.
  3. Bug fixes only need to be backported to a small number of designated LTS releases.
  4. Extra tests (e.g. upgrade tests for non-subsequent versions) only need to be executed against a small number of designated LTS releases.

At the same time, the following is true for the LTS

  1. Minimal set of feature additions and other changes that can easily be applied, reducing the risk of functional regressions and bugs

However, feature addition to LTS should be rare. Adding a new API in go shim would also result in adding the same APIs for java and node shim. Hence, this cannot be backported to LTS easily. Any new feature whether it is big/small should be first added to the master.

As you mentioned correctly i used an existing and old JIRA ticket to provide this addition. Today i created a new one FAB-17919 and linked any other relevant issues to share your thoughts, concerns, and start the discussion from a fresh base.

I didn't mean that. Given a story/task, we need to use the oldest JIRA and mark all others as duplicate. Hence, let's discuss on the oldest JIRA itself.

@lindluni
Copy link
Contributor

To expand on Senthil's statement, one of the reasons changes are made to master before a release branch is that it allows us to evaluate the stability of the change before backporting it to a prior release. This is important and something we generally don't waiver on, as disrupting a release branch can have huge impacts to developers dependent on our releases.

Our LTS strategy as updated this quarter is now a "bug only" strategy. HIstorically we haven't followed this rule, and it has had consequences for people running our software. So defined a true LTS strategy and are now hyper-sensitive to the fact there are people running our software in production. With that we agreed, LTS releases are now bug-only and new features will not be backported to release-1.4.

That being said, regardless of the change, unless it was a bug only impacting release-1.4, our policy is the change must land in master before being evaluated for backport to a prior release.

@chanioxaris
Copy link
Author

Thanks @cendhu and @btl5037 for your input. Totally understandable about your policy on new features. I believe we should consider this PR as not valid and start a discussion on JIRA ticker or Rocket chat, as @cendhu propose, on how to approach this feature.

Also more changes are required and new API methods need to be added on at least two interfaces (QueryExecutor, DB) and chaincode handler.

@cendhu
Copy link
Contributor

cendhu commented May 26, 2020

@chanioxaris Given that, we would not merge this feature in v1.4, can you please close this PR for now?

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.

None yet

4 participants