On demand OS transport #1635

lebdron · 2018-08-08T19:44:24Z

Description of the Change

Add gRPC client and server for on demand ordering service

Add gRPC mock class generation
Unit tests for both client and server! No need to run real server with threads for unit tests
Reuse server notification interface in client - each peer is represented with corresponding notification interface

Note

Factory is not covered with unit tests due to necessary hardcoded creation of real network channel. It will be covered by integration tests in next pull requests
mock_stream.h is copied from gRPC repo with an addition of mock method, because it is not provided after installation

muratovv · 2018-08-13T08:46:00Z

cmake/functions.cmake

@@ -82,10 +82,13 @@ endfunction()
 function(compile_proto_only_grpc_to_cpp PROTO)
  string(REGEX REPLACE "\\.proto$" ".grpc.pb.h" GEN_GRPC_PB_HEADER ${PROTO})
  string(REGEX REPLACE "\\.proto$" ".grpc.pb.cc" GEN_GRPC_PB ${PROTO})
+  if(TESTING)
+    set(GENERATE_MOCKS "generate_mock_code=true:")


It doesn't clear that this line is related to grpc mocks, maybe add a comment about it?

muratovv · 2018-08-13T09:51:08Z

irohad/ordering/impl/on_demand_os_client_grpc.cpp

+#include "backend/protobuf/proposal.hpp"
+#include "network/impl/grpc_channel_builder.hpp"
+
+using namespace iroha::ordering::transport;


It seems like good policy: use only one namespace in the cpp file which is provided by the header file.

This is ofc not an issue to your PR but a topic for a discussion.

muratovv · 2018-08-13T10:26:38Z

irohad/ordering/impl/on_demand_os_client_grpc.hpp

+            std::function<OnDemandOsClientGrpc::TimepointType()> time_provider,
+            OnDemandOsClientGrpc::TimeoutType timeout);
+
+        std::unique_ptr<OdOsNotification> create(


It seems a bit unobvious why we use factory here? Why are we required to use both: ctor and factory? I suggest putting the answer to the comment here.

muratovv · 2018-08-13T10:27:46Z

irohad/ordering/impl/on_demand_os_server_grpc.cpp

+    std::shared_ptr<OdOsNotification> ordering_service)
+    : ordering_service_(ordering_service) {}
+
+// proto::OnDemandOrdering::Service


Seems like misleading comment

Why? Methods below are implementation of proto::OnDemandOrdering::Service interface, so this line shows that property. Similar approach is used in client: https://github.com/hyperledger/iroha/blob/83bf44f91448474207c9fc99a837fef83be44bc0/irohad/ordering/impl/on_demand_os_client_grpc.hpp#L33-L38

Maybe write it more explicitly. Such as

// ----| proto::OnDemandOrdering::Service |-----

fitted for the line?

muratovv · 2018-08-13T10:29:20Z

irohad/ordering/impl/on_demand_os_server_grpc.cpp

+    const proto::TransactionsCollection *request,
+    ::google::protobuf::Empty *response) {
+  OdOsNotification::CollectionType transactions;
+  for (const auto &i : request->transactions()) {


Rename i with tx or something simular.

muratovv · 2018-08-13T10:32:05Z

irohad/ordering/impl/on_demand_os_client_grpc.hpp

+
+       private:
+        std::function<OnDemandOsClientGrpc::TimepointType()> time_provider_;
+        std::chrono::milliseconds timeout_;


add semantic for the variable, in comments or in the name.

muratovv · 2018-08-13T10:35:24Z

irohad/ordering/impl/on_demand_os_server_grpc.hpp

+        explicit OnDemandOsServerGrpc(
+            std::shared_ptr<OdOsNotification> ordering_service);
+
+        // proto::OnDemandOrdering::Service


comment, also, looks misleading

muratovv · 2018-08-13T10:39:30Z

test/framework/mock_stream.h

+#ifndef GRPCPP_TEST_MOCK_STREAM_H
+#define GRPCPP_TEST_MOCK_STREAM_H
+
+#include <stdint.h>


What is the reason to use stdinthere?

This is a copy of https://github.com/grpc/grpc/blob/34e8e0a6400d8b529125a3b83ec1facf71acf99b/include/grpcpp/test/mock_stream.h .
I think it is because of uint32_t here: https://github.com/hyperledger/iroha/blob/83bf44f91448474207c9fc99a837fef83be44bc0/test/framework/mock_stream.h#L73

So why we can't remove it?

Because uint32_t is defined in stdint.h

muratovv · 2018-08-13T10:45:20Z

test/framework/mock_stream.h

+  MOCK_METHOD0_T(WritesDone, bool());
+};
+
+/// TODO: We do not support mocking an async RPC for now.


Maybe create a task for that. Also, what is the reason for that?

mock_stream.h is copied from gRPC repo with an addition of mock method, because it is not provided after installation

I did not want to make any modifications to this file. Do you suggest removing the todo?

In this case, yes. I think this todo will mislead readers of the file.

kamilsa · 2018-08-10T13:06:00Z

irohad/ordering/impl/on_demand_os_client_grpc.cpp

+void OnDemandOsClientGrpc::onTransactions(CollectionType &&transactions) {
+  proto::TransactionsCollection message;
+  for (auto &transaction : transactions) {
+    *message.add_transactions() = std::move(


why to move rvalue?

The moment when rvalue gets a name (transactions in this case), it becomes an lvalue. So it has to be called with std::move to become an rvalue again.

kamilsa · 2018-08-10T13:08:11Z

irohad/ordering/impl/on_demand_os_client_grpc.cpp

+    return boost::none;
+  }
+  return ProposalType{std::make_unique<shared_model::proto::Proposal>(
+      std::move(response.proposal()))};


again, I think move on function call is redundant

kamilsa · 2018-08-13T08:39:17Z

irohad/ordering/impl/on_demand_os_client_grpc.cpp

+  proto::TransactionsCollection message;
+  for (auto &transaction : transactions) {
+    *message.add_transactions() = std::move(
+        static_cast<shared_model::proto::Transaction *>(transaction.get())


use std::static_pointer_cast instead

It works only for shared pointers, while transaction is a unique pointer.
https://en.cppreference.com/w/cpp/memory/shared_ptr/pointer_cast
https://github.com/hyperledger/iroha/blob/83bf44f91448474207c9fc99a837fef83be44bc0/irohad/ordering/on_demand_os_transport.hpp#L64-L73

Changes done

Signed-off-by: Andrei Lebedev <[email protected]> Refactor with new async call Signed-off-by: Andrei Lebedev <[email protected]>

* BFT OS trunk * On demand OS transport (#1635) * On demand OS connection manager (#1645) * On-demand ordering gate and related fixes (#1675) Signed-off-by: Andrei Lebedev <[email protected]>

lebdron requested review from muratovv and kamilsa August 8, 2018 19:44

muratovv previously requested changes Aug 13, 2018

View reviewed changes

kamilsa suggested changes Aug 14, 2018

View reviewed changes

kamilsa approved these changes Aug 14, 2018

View reviewed changes

lebdron requested a review from muratovv August 14, 2018 15:24

muratovv approved these changes Aug 15, 2018

View reviewed changes

lebdron changed the base branch from trunk/bft_os to trunk/bft-os August 16, 2018 13:16

lebdron force-pushed the feature/bft-os-transport branch from 06899b7 to f3bf2c2 Compare August 16, 2018 13:16

lebdron force-pushed the trunk/bft-os branch from 1f6c56e to ed7b375 Compare August 17, 2018 13:54

Od Os transport client and server

0e639e6

Signed-off-by: Andrei Lebedev <[email protected]> Refactor with new async call Signed-off-by: Andrei Lebedev <[email protected]>

lebdron force-pushed the feature/bft-os-transport branch from f3bf2c2 to 0e639e6 Compare August 17, 2018 16:06

lebdron merged commit 7ab8e34 into trunk/bft-os Aug 18, 2018

lebdron deleted the feature/bft-os-transport branch August 19, 2018 13:44

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

On demand OS transport #1635

On demand OS transport #1635

lebdron commented Aug 8, 2018 •

edited

Loading

muratovv Aug 13, 2018

muratovv Aug 13, 2018

muratovv Aug 13, 2018

muratovv Aug 13, 2018

lebdron Aug 13, 2018

muratovv Aug 14, 2018 •

edited

Loading

muratovv Aug 13, 2018

muratovv Aug 13, 2018

muratovv Aug 13, 2018

muratovv Aug 13, 2018

lebdron Aug 13, 2018

muratovv Aug 14, 2018

lebdron Aug 14, 2018

muratovv Aug 13, 2018

lebdron Aug 13, 2018 •

edited

Loading

muratovv Aug 14, 2018

kamilsa Aug 10, 2018

lebdron Aug 14, 2018

kamilsa Aug 10, 2018

kamilsa Aug 13, 2018

lebdron Aug 14, 2018

On demand OS transport #1635

On demand OS transport #1635

Conversation

lebdron commented Aug 8, 2018 • edited Loading

Description of the Change

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron commented Aug 8, 2018 •

edited

Loading

muratovv Aug 14, 2018 •

edited

Loading

lebdron Aug 13, 2018 •

edited

Loading