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

[fix] #2963: Queue remove transactions correctly #3035

Merged
merged 2 commits into from
Dec 29, 2022

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Dec 22, 2022

Description of the Change

  • Add queue size to metrics;
  • Fix Queue to correctly remove transactions;

Issue

Closes #2963.

Benefits

  • No infinite growth of the transaction queue;
  • More diagnostic information in the /status response.

Possible Drawbacks

None.

Usage Examples or Tests [optional]

Queue size field in the status response:

curl http://127.0.0.1:8180/status

Alternate Designs [optional]

Multi-signatures are still not handled correctly by the queue, i would suggest fix this in the separate PR.

@Erigara Erigara self-assigned this Dec 22, 2022
@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Dec 22, 2022
@Erigara Erigara added the Bug Something isn't working label Dec 22, 2022
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #3035 (043dd97) into iroha2-dev (a4d5c9f) will increase coverage by 1.22%.
The diff coverage is 65.58%.

@@              Coverage Diff               @@
##           iroha2-dev    #3035      +/-   ##
==============================================
+ Coverage       62.33%   63.55%   +1.22%     
==============================================
  Files             169      170       +1     
  Lines           31218    32864    +1646     
==============================================
+ Hits            19459    20886    +1427     
- Misses          11759    11978     +219     
Impacted Files Coverage Δ
cli/src/main.rs 1.09% <0.00%> (ø)
cli/src/torii/mod.rs 27.65% <ø> (ø)
client/src/client.rs 39.91% <0.00%> (ø)
client_cli/src/main.rs 0.24% <0.00%> (-0.01%) ⬇️
config/base/src/lib.rs 91.57% <ø> (+55.21%) ⬆️
config/src/lib.rs 33.33% <ø> (ø)
config/src/path.rs 0.00% <0.00%> (ø)
core/src/smartcontracts/isi/block.rs 85.71% <ø> (-3.18%) ⬇️
data_model/src/block_value.rs 100.00% <ø> (ø)
data_model/src/lib.rs 58.45% <ø> (+8.45%) ⬆️
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

core/src/queue.rs Outdated Show resolved Hide resolved
@mversic
Copy link
Contributor

mversic commented Dec 22, 2022

something's wrong with issue id in the title. It actually references a PR instead. Probably a github issue? But even the Close #num references the PR, you can fix that

@Erigara
Copy link
Contributor Author

Erigara commented Dec 22, 2022

something's wrong with issue id in the title. It actually references a PR instead. Probably a github issue? But even the Close #num references the PR, you can fix that

oh, sure

@Erigara Erigara changed the title [fix] #2978: Queue remove transactions correctly [fix] #2963: Queue remove transactions correctly Dec 22, 2022
@appetrosyan appetrosyan force-pushed the iroha2-dev branch 3 times, most recently from 2b18ee8 to 3e1fb30 Compare December 23, 2022 12:21
@QuentinI QuentinI self-assigned this Dec 26, 2022
@Erigara Erigara marked this pull request as draft December 26, 2022 08:51
@Erigara Erigara force-pushed the fix_queue branch 2 times, most recently from 3ca0ffe to a1e6e42 Compare December 26, 2022 15:54
@Erigara Erigara marked this pull request as ready for review December 26, 2022 15:55
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Show resolved Hide resolved
QuentinI
QuentinI previously approved these changes Dec 27, 2022
Copy link
Contributor

@QuentinI QuentinI left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long, wanted to make sure I understood what's going on.

core/src/queue.rs Show resolved Hide resolved
core/src/queue.rs Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
core/src/queue.rs Outdated Show resolved Hide resolved
@Erigara Erigara merged commit d51c1c6 into hyperledger:iroha2-dev Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working 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