[LEDGER] replace Metadata in query API with Limit and Bookmark #1024

cendhu · 2020-04-08T14:24:44Z

Type of change

Improvement (improvement to code)

Description

The GetStateRangeScanIteratorWithMetadata() at both stateleveldb and statecouchdb receive metadata such as pagination limit.
The ExecuteQueryWithMetadata() at statecouchdb receives metadata such as the pagination bookmark and pagination limit.
The ValidateRangeMetadata() and ValidateQueryMetadata() validate the received metadata for the respective query.

However, the term Metadata is confusing with the statemetadata which stores the key-based endorsement. The API SetXXXMetadata() and GetXXXMetadata() are associated with the per state metadata. To avoid this confusion, we replace Metadata in range query and execute query API with Pagination to be explicit. As we have removed the metadata, we also removed ValidateRangeMetadata() and ValidateQueryMetadata() as they are not needed anymore.

GetStateRangeScanIteratorWithMetadata() -> GetStateRangeScanIteratorWithPagination()
ExecuteQueryWithMetadata() -> ExecuteQueryWithPagination()

Related issues

FAB-17721

manish-sethi

I certainly find using "Options" instead of "Metadata" more meaningful but inconsistent naming causes more confusion. I would prefer to apply this change to this function ExecuteQueryWithMetadata as well.
Moving a tiny piece of code to internal package because it is used in two packages is not that useful - particularly, when you have a similar function for rich queries still sitting statecouchdb package. I find keeping the related code as close as possible more useful.
wdyt?

cendhu · 2020-04-09T02:09:44Z

While I was changing the code, I did change Metadata to Options for the ExecuteQuery as well. Then, I reverted the changes thinking that it can be done in a separate PR and concentrate on range queries only in this PR. Hence, let's do it in the next PR.
If we need to keep it close, we need to duplicate the code. I don't think that either stateleveldb or statecouchdb should import statedb for a utility function if it cannot import an internal package. In fact, in my initial changes, I tried to remove the map[string]interface{} arguments from the range query as we are only passing the limit for now and extending this function later would not break any backward compatibility. IMO, it would be good to code for a feature that we currently support today. If we need to remove the introduced internal package, I would prefer to replace the map argument with the limit parameter and avoid this validation code altogether. However, in this case, I am not sure whether we can name it Options instead Limit makes more sense. Otherwise, we can introduce a new struct named queryOptions and introduce bookmark and limit. Then, use it for both types of queries.

What do you think?

cendhu · 2020-04-11T02:57:53Z

@Mergifyio rebase

mergify · 2020-04-11T02:58:25Z

@cendhu is not allowed to run commands

denyeart · 2020-04-11T17:29:47Z

I would prefer we simply change "WithMetadata" to "WithPagination" or "WithOptions". I think that would be more expressive, cleaner, and a smaller change, than saying things like "WithBookmarkAndLimit". What if a database has 10 query options, you wouldn't want the API itself to mention all 10 of the query options.

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

cendhu · 2020-04-12T02:38:49Z

I would prefer we simply change "WithMetadata" to "WithPagination" or "WithOptions". I think that would be more expressive, cleaner, and a smaller change, than saying things like "WithBookmarkAndLimit". What if a database has 10 query options, you wouldn't want the API itself to mention all 10 of the query options.

I can use WithPagination then. We added this pagination support around May 2018 and we passed a map instead of the parameters directly. We then had separate map validation code -- one for the executeQuery and another for the range query. The range query validation code was placed in the statedb package and imported by both statecouchdb and stateleveldb packages to avoid code duplication. We did all this to support that 10 query options. In two years, we never supported any other options other than the pagination option. The map validation code and related test code are not justified. This PR has removed them and reduced 200 LOC. When we support that 10 query options, we can pass an option. For now, I can change WithMetadata to WithPagination instead of WithLimit and WithBookmarkAndLimit.

manish-sethi

A few comment, mainly on the chaincode side.

core/ledger/kvledger/txmgmt/statedb/stateleveldb/stateleveldb.go

core/chaincode/handler.go

manish-sethi · 2020-04-14T20:09:58Z

I would prefer we simply change "WithMetadata" to "WithPagination" or "WithOptions". I think that would be more expressive, cleaner, and a smaller change, than saying things like "WithBookmarkAndLimit". What if a database has 10 query options, you wouldn't want the API itself to mention all 10 of the query options.

I can use WithPagination then. We added this pagination support around May 2018 and we passed a map instead of the parameters directly. We then had separate map validation code -- one for the executeQuery and another for the range query. The range query validation code was placed in the statedb package and imported by both statecouchdb and stateleveldb packages to avoid code duplication. We did all this to support that 10 query options. In two years, we never supported any other options other than the pagination option. The map validation code and related test code are not justified. This PR has removed them and reduced 200 LOC. When we support that 10 query options, we can pass an option. For now, I can change WithMetadata to WithPagination instead of WithLimit and WithBookmarkAndLimit.

@denyeart, @cendhu - IMO, there is no need to have two variations for the queries. Simply the basic APIs (i.e., neither WithPagination nor WithMetadata/WithOptions) can take additional options. Even, in the current code the Basic API simply call the other one with default options. We cannot help it at the chaincode layer for compatibility reason but that should not be dragged to the last mile in the code.

cendhu · 2020-04-15T02:23:16Z

@manish-sethi Valid point. I agree with you. I have created a separate JIRA FAB-17754 to address this once we decide on the following. To define and import option structs (one per query type--range and executeQuery), we need to agree on a location where we can keep all types exported by the ledger package to be used by both non-ledger packages and the ledger package. As it might take a bit longer to decide on that, we can merge this PR for now.

cendhu · 2020-04-15T04:06:04Z

@btl5037 2 second wait time for CouchDB index creation does not seem to help 😞 We might need to increase the wait time a bit. I hit the flakes twice in this PR.

lindluni · 2020-04-15T04:10:00Z

@btl5037 2 second wait time for CouchDB index creation does not seem to help 😞 We might need to increase the wait time a bit. I hit the flakes twice in this PR.

Yea. We hit it in 3 other PRs today as well :(

cendhu · 2020-04-15T04:25:38Z

@btl5037 Do we use SSD or HDD? These tests run in parallel right? If it is SSD, 2 seconds should be adequate and surprising why it fails. If it is HDD, we need to increase the time. Btw, what is the allocated #vCPUs? The CouchDB also consumes 3x higher CPU than GoLevelDB. Even with SSD, if we have allocated lower #vCPUs and run tests in parallel, I am sure that 2 seconds is not adequate.

lindluni · 2020-04-15T04:29:51Z

2vCPU and 4GB running on SSD

manish-sethi

LGTM. A couple nit-picking comments though, that you may fix when resolving conflicts.

manish-sethi · 2020-04-16T04:11:28Z

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

-	// Get the internalQueryLimit from core.yaml
+// pageSize limits the number of results returned
+func (vdb *VersionedDB) GetStateRangeScanIteratorWithPagination(namespace string, startKey string, endKey string, pageSize int32) (statedb.QueryResultsIterator, error) {
+	logger.Debugf("Entering GetStateRangeScanIteratorWithOptions namespace: %s  startKey: %s  endKey: %s  options: %d", namespace, startKey, endKey, pageSize)


out of sync with new function name and parameter names

manish-sethi · 2020-04-16T04:11:43Z

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

-	logger.Debugf("Entering ExecuteQueryWithMetadata  namespace: %s,  query: %s,  metadata: %v", namespace, query, metadata)
+// ExecuteQueryWithPagination implements method in VersionedDB interface
+func (vdb *VersionedDB) ExecuteQueryWithPagination(namespace, query, bookmark string, pageSize int32) (statedb.QueryResultsIterator, error) {
+	logger.Debugf("Entering ExecuteQueryWithMetadata  namespace: %s,  query: %s,  metadata: %d", namespace, query, pageSize)


out of sync with new function name and parameter names

The range query receives metadata such as pagination limit. The execute query at couchdb receives metadata such as the pagination bookmark and pagination limit. ValidateRangeMetadata() and ValidateQueryMetadata() validate the received metadata. However, the term metadata is confusing with the statemetadata which stores the key-based endorsement. Hence, we replace Metadata in range and execute query API with Pagination. As we have removed the metadata map, we also removed ValidateRangeMetadata() and ValidateQueryMetadata() as they are not needed anymore. Signed-off-by: senthil <cendhu@gmail.com>

cendhu requested a review from a team as a code owner April 8, 2020 14:24

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch from cf629fe to d347d13 Compare April 8, 2020 14:31

manish-sethi requested changes Apr 8, 2020

View reviewed changes

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch from d347d13 to a706c6e Compare April 9, 2020 16:03

cendhu changed the title ~~mv rangequery options validation to ledger/internal~~ replace Metadata in query API with Limit and Bookmark Apr 9, 2020

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch from a706c6e to f15dd85 Compare April 9, 2020 16:44

cendhu changed the title ~~replace Metadata in query API with Limit and Bookmark~~ [LEDGER] replace Metadata in query API with Limit and Bookmark Apr 9, 2020

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch 2 times, most recently from a47974a to ebc508d Compare April 10, 2020 15:22

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch from ebc508d to 5ef925b Compare April 11, 2020 03:23

denyeart reviewed Apr 11, 2020

View reviewed changes

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

denyeart reviewed Apr 11, 2020

View reviewed changes

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

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch 2 times, most recently from 4378fa4 to 2111832 Compare April 12, 2020 12:47

cendhu requested a review from manish-sethi April 12, 2020 12:51

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch from 2111832 to b7d546b Compare April 13, 2020 01:57

manish-sethi requested changes Apr 14, 2020

View reviewed changes

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

core/chaincode/handler.go Outdated Show resolved Hide resolved

core/chaincode/handler.go Outdated Show resolved Hide resolved

manish-sethi previously approved these changes Apr 16, 2020

View reviewed changes

cendhu dismissed manish-sethi’s stale review via 4d52a39 April 17, 2020 10:50

cendhu force-pushed the mv-range-query-parameter-validation-to-internals branch from 756bd08 to 4d52a39 Compare April 17, 2020 10:50

cendhu requested review from manish-sethi and denyeart April 17, 2020 12:02

manish-sethi approved these changes Apr 18, 2020

View reviewed changes

manish-sethi merged commit b17b6dd into hyperledger:master Apr 18, 2020

cendhu deleted the mv-range-query-parameter-validation-to-internals branch April 18, 2020 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LEDGER] replace Metadata in query API with Limit and Bookmark #1024

[LEDGER] replace Metadata in query API with Limit and Bookmark #1024

cendhu commented Apr 8, 2020 •

edited

Loading

manish-sethi left a comment

cendhu commented Apr 9, 2020 •

edited

Loading

cendhu commented Apr 11, 2020

mergify bot commented Apr 11, 2020

denyeart commented Apr 11, 2020

cendhu commented Apr 12, 2020 •

edited

Loading

manish-sethi left a comment

manish-sethi commented Apr 14, 2020

cendhu commented Apr 15, 2020

cendhu commented Apr 15, 2020

lindluni commented Apr 15, 2020

cendhu commented Apr 15, 2020 •

edited

Loading

lindluni commented Apr 15, 2020

manish-sethi left a comment

manish-sethi Apr 16, 2020

cendhu Apr 17, 2020

manish-sethi Apr 16, 2020

cendhu Apr 17, 2020

[LEDGER] replace Metadata in query API with Limit and Bookmark #1024

[LEDGER] replace Metadata in query API with Limit and Bookmark #1024

Conversation

cendhu commented Apr 8, 2020 • edited Loading

Type of change

Description

Related issues

manish-sethi left a comment

Choose a reason for hiding this comment

cendhu commented Apr 9, 2020 • edited Loading

cendhu commented Apr 11, 2020

mergify bot commented Apr 11, 2020

denyeart commented Apr 11, 2020

cendhu commented Apr 12, 2020 • edited Loading

manish-sethi left a comment

Choose a reason for hiding this comment

manish-sethi commented Apr 14, 2020

cendhu commented Apr 15, 2020

cendhu commented Apr 15, 2020

lindluni commented Apr 15, 2020

cendhu commented Apr 15, 2020 • edited Loading

lindluni commented Apr 15, 2020

manish-sethi left a comment

Choose a reason for hiding this comment

manish-sethi Apr 16, 2020

Choose a reason for hiding this comment

cendhu Apr 17, 2020

Choose a reason for hiding this comment

manish-sethi Apr 16, 2020

Choose a reason for hiding this comment

cendhu Apr 17, 2020

Choose a reason for hiding this comment

cendhu commented Apr 8, 2020 •

edited

Loading

cendhu commented Apr 9, 2020 •

edited

Loading

cendhu commented Apr 12, 2020 •

edited

Loading

cendhu commented Apr 15, 2020 •

edited

Loading