Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

ITF fixes related to BFT OS #1826

Merged
merged 20 commits into from
Nov 9, 2018
Merged

ITF fixes related to BFT OS #1826

merged 20 commits into from
Nov 9, 2018

Conversation

lebdron
Copy link
Contributor

@lebdron lebdron commented Nov 1, 2018

Description of the Change

  • Fix deadlock on gRPC server destruction when a call is pending
  • Store transactions for corresponding round in OS if they are send in closed round
  • Restrict lifetime of block query in simulator to prevent destruction locks
  • Disable send tx sequence tests since they cannot be used with individual batch propagation and without caching

Benefits

  • Working ITF tests 馃帀

Possible Drawbacks

  • System tests still not working

@lebdron
Integrate on-demand OS with fixes
8b2ca7c 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Merge branch 'dev' into feature/bft-os-integration
57954ca 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Merge branch 'dev' into feature/bft-os-integration
54876d0 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Fix compilation
8cfe2d6 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Fix round number on commit
5b91959 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Comment out integration tests and fix gate test
02c28a1 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Use hashes from storage if present
52e2dcb 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Add missing header
c37f68a 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Merge branch 'dev' into feature/bft-os-integration
64d362d 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Pass factory as shared_ptr
644f872 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Add docs, fix possible overflow
ce29a3a 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Add class doc, clang-format
ba53a2d 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
ITF fixes related to BFT OS
81cd43f 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
l4l
l4l previously requested changes Nov 1, 2018
View reviewed changes
irohad/ordering/impl/on_demand_ordering_service_impl.cpp
}
std::for_each(batches.begin(), batches.end(), [&it](auto &obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

auto && ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

@l4l l4l Nov 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

since move is called on the line below

Copy link
Contributor Author

@lebdron lebdron Nov 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So how does universal reference help here? It is possible to move from an lvalue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, sure, even compiler agree. Though semantically it's more intuitive use universal imo, not a big deal really

irohad/simulator/impl/simulator.cpp Outdated
@@ -70,6 +70,7 @@ namespace iroha {
log_->warn("Could not fetch last block: " + e->error);
return;
}
block_query_opt = boost::none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use braces instead (aka { & }) for dealing with lifetimes, it's easier to understand stuff like that

irohad/main/server_runner.cpp
const std::chrono::system_clock::time_point &deadline) {
if (serverInstance_) {
serverInstance_->Shutdown(deadline);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be write something at the else branch? It's not that normal flow, might be handy

Copy link
Contributor

Choose a reason for hiding this comment

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

What will you do inside else branch?) there will be nothing to shut down or destroy in case of enclosing object was not fully initialized for some reason

Copy link
Contributor

Choose a reason for hiding this comment

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

just logging :)

test/framework/integration_framework/test_irohad.hpp
void terminate(const std::chrono::system_clock::time_point &deadline) {
if (internal_server) {
internal_server->shutdown(deadline);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the else branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, nothing to terminate if there is no internal server.

@l4l l4l added needs-review pr awaits review from maintainers bug bug/defect that was reproduced by maintainers ordering labels Nov 1, 2018
@lebdron
Merge branch 'trunk/bft-os-integration' into feature/bft-os-integration
6306d41 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Fix merge conflicts
fc7a463 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
igor-egorov
igor-egorov approved these changes Nov 8, 2018
View reviewed changes
Copy link
Contributor

@igor-egorov igor-egorov left a comment

Choose a reason for hiding this comment

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

LGTM

Build has to be fixed

@lebdron
Review fixes
e20a8e7 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Review fixes
b3a2361 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron
Merge branch 'feature/bft-os-integration' into refactor/bft-os-itf-tests
9b4165d
@lebdron
Review fixes
012b1e2 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron lebdron dismissed l4l鈥檚 stale review November 9, 2018 06:45

Issues addressed

@lebdron lebdron requested a review from l4l November 9, 2018 06:45
@l4l l4l removed the needs-review pr awaits review from maintainers label Nov 9, 2018
@lebdron lebdron changed the base branch from feature/bft-os-integration to trunk/bft-os-integration November 9, 2018 15:17
@lebdron lebdron changed the base branch from trunk/bft-os-integration to feature/bft-os-integration November 9, 2018 15:24
@lebdron
Merge branch 'trunk/bft-os-integration' into refactor/bft-os-itf-tests
2fa9ccc 
Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron lebdron changed the base branch from feature/bft-os-integration to trunk/bft-os-integration November 9, 2018 15:29
@lebdron lebdron merged commit 37bb144 into trunk/bft-os-integration Nov 9, 2018
@lebdron lebdron deleted the refactor/bft-os-itf-tests branch November 9, 2018 15:58
@kamilsa kamilsa mentioned this pull request Dec 12, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@l4l l4l l4l approved these changes

@igor-egorov igor-egorov igor-egorov approved these changes

Assignees
No one assigned
Labels
bug bug/defect that was reproduced by maintainers ordering
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lebdron @l4l @igor-egorov