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

[feature] #3900: fetch_size query parameter #4016

Merged

Conversation

Arjentix
Copy link
Contributor

Description

Linked issue

  • fetch_size is no more a configuration parameter but a query parameter. Clients have to send it as a query parameter.
  • Added integration test

Benefits

  • Clients can configure how big batches they want

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests integration test for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@Arjentix Arjentix added Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST labels Oct 26, 2023
@Arjentix Arjentix self-assigned this Oct 26, 2023
@coveralls
Copy link

coveralls commented Oct 26, 2023

Pull Request Test Coverage Report for Build 6811282567

  • 19 of 102 (18.63%) changed or added relevant lines in 11 files are covered.
  • 6915 unchanged lines in 128 files lost coverage.
  • Overall coverage decreased (-4.0%) to 55.39%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cli/src/torii/mod.rs 0 1 0.0%
client/src/query_builder.rs 0 6 0.0%
core/src/smartcontracts/wasm.rs 4 10 40.0%
core/src/query/cursor.rs 0 7 0.0%
cli/src/torii/routing.rs 0 10 0.0%
client/src/client.rs 1 11 9.09%
core/src/query/store.rs 1 12 8.33%
data_model/src/query/mod.rs 5 21 23.81%
smart_contract/src/lib.rs 0 16 0.0%
Files with Coverage Reduction New Missed Lines %
config/base/derive/src/view.rs 1 99.37%
config/src/block_sync.rs 1 95.0%
config/src/network.rs 1 93.75%
config/src/torii.rs 1 95.45%
config/src/wasm.rs 1 87.5%
core/src/smartcontracts/isi/block.rs 1 87.5%
config/src/kura.rs 2 79.41%
config/src/lib.rs 2 0.0%
ffi/src/option.rs 2 71.43%
config/src/genesis.rs 3 72.92%
Totals Coverage Status
Change from base Build 5423219773: -4.0%
Covered Lines: 22142
Relevant Lines: 39975

💛 - Coveralls

@Erigara Erigara self-assigned this Oct 27, 2023
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Imo we should limit maximum size of fetch_size.

@mversic mversic self-assigned this Oct 30, 2023
@Arjentix
Copy link
Contributor Author

Imo we should limit maximum size of fetch_size.

But how? Using another config parameter?

@Erigara
Copy link
Contributor

Erigara commented Oct 31, 2023

But how? Using another config parameter?

Or hardcode, however we can leave this as is for now.

Erigara
Erigara previously approved these changes Oct 31, 2023
client/src/client.rs Show resolved Hide resolved
data_model/src/query/mod.rs Outdated Show resolved Hide resolved
@Erigara Erigara dismissed their stale review October 31, 2023 07:08

misclick...

Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

Merging only after a PR to update Torii Endpoints page is created.

upd: sorry for ultimate tone, just tired from having outdated docs.

@0x009922 0x009922 added the api-changes Changes in the API for client libraries label Oct 31, 2023
@Arjentix
Copy link
Contributor Author

Imo we should limit maximum size of fetch_size.

But how? Using another config parameter?

Or hardcode, however we can leave this as is for now.

@mversic , what do you think about that?

@Arjentix
Copy link
Contributor Author

Merging only after a PR to update Torii Endpoints page is created.

@0x009922 , is it iroha2-docs repo?

@mversic
Copy link
Contributor

mversic commented Oct 31, 2023

Imo we should limit maximum size of fetch_size.

But how? Using another config parameter?

Or hardcode, however we can leave this as is for now.

@mversic , what do you think about that?

I think it's ok to just hardcode

@Arjentix Arjentix force-pushed the fetch_size_per_query branch 3 times, most recently from a6d2259 to 2c76373 Compare October 31, 2023 19:22
@0x009922
Copy link
Contributor

0x009922 commented Nov 1, 2023

Merging only after a PR to update Torii Endpoints page is created.

@0x009922 , is it iroha2-docs repo?

@Arjentix, yes. At the bottom of the page you might find "Edit this page on GitHub" link for convenience.

@Arjentix
Copy link
Contributor Author

Arjentix commented Nov 2, 2023

@0x009922 , created hyperledger-iroha/iroha-2-docs#425

mversic
mversic previously approved these changes Nov 6, 2023
@mversic
Copy link
Contributor

mversic commented Nov 6, 2023

Imo we should limit maximum size of fetch_size.

But how? Using another config parameter?

Or hardcode, however we can leave this as is for now.

@mversic , what do you think about that?

I think it's ok to just hardcode

I would like to backport this into RC20. But in the backport we have to leave in the config intact since removing it would break backwards compatibility

@mversic mversic requested a review from 0x009922 November 6, 2023 06:42
@Arjentix
Copy link
Contributor Author

Arjentix commented Nov 7, 2023

I would like to backport this into RC20. But in the backport we have to leave in the config intact since removing it would break backwards compatibility

Ok, will do in separate PR

@Arjentix
Copy link
Contributor Author

Arjentix commented Nov 7, 2023

@mversic , I tried and turns out this PR uses a lot from #3958 which is not merged into stable.

Also this seems to be a feature, while AFAIK only bug-fix backports make sence

@mversic
Copy link
Contributor

mversic commented Nov 8, 2023

Also this seems to be a feature, while AFAIK only bug-fix backports make sence

ideally, only bugfixes would be backported. However, this feature was requested by Orillion.
Nvm, if not possible

Erigara
Erigara previously approved these changes Nov 8, 2023
Signed-off-by: Daniil Polyakov <arjentix@gmail.com>
@Arjentix Arjentix merged commit b22ebc4 into hyperledger-iroha:iroha2-dev Nov 9, 2023
10 checks passed
@Arjentix Arjentix deleted the fetch_size_per_query branch November 9, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries Enhancement New feature or request iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants