Feature/tx status bus #1575

l4l · 2018-07-18T22:34:43Z

Description of the Change

Add StatusBus interface
Add StatusBusImpl based on manual working with std::thread
Use StatusBus for sharing tx statuses among CommandService and TransactionProcessor
Introduce boost::thread dependency for boost::barrier
ITF::sendTx synchronize is based on StatusBus

Benefits

Cleaner architecture, some work delegate to other unit, lesser RC issues in tests

Possible Drawbacks

None

nickaleks · 2018-07-20T11:46:56Z

irohad/torii/command_service.hpp

@@ -29,6 +29,7 @@
 #include "endpoint.pb.h"
 #include "logger/logger.hpp"
 #include "torii/processor/transaction_processor.hpp"
+#include "torii/status_bus_impl.hpp"


status_bus.hpp?

nickaleks · 2018-07-20T11:47:36Z

irohad/torii/command_service.hpp

-    void addTxToCacheAndLog(const std::string &who,
-                            const shared_model::crypto::Hash &hash,
-                            const iroha::protocol::ToriiResponse &response);
+    void pushStatus(const std::string &who,


Add comment please

nickaleks · 2018-07-20T11:59:28Z

irohad/torii/impl/status_bus_impl.cpp

+    }
+
+    void StatusBusImpl::update() {
+      while (not q_.empty() and q_.try_pop(obj_)) {


You do not need to check for emptiness, try_pop returns false if the queue is empty.

nickaleks · 2018-07-20T12:03:55Z

irohad/torii/processor/impl/transaction_processor_impl.cpp

+        std::shared_ptr<iroha::torii::StatusBus> status_bus)
+        : pcs_(std::move(pcs)),
+          mst_processor_(std::move(mst_processor)),
+          status_bus_(status_bus) {


Why not move here?

nickaleks · 2018-07-20T12:06:24Z

irohad/torii/status_bus_impl.hpp

+#include <mutex>
+#include <thread>
+
+#include <tbb/concurrent_queue.h>


Add empty line after include please

nickaleks · 2018-07-23T13:00:11Z

irohad/torii/impl/command_service.cpp

@@ -155,7 +150,7 @@ namespace torii {
                // Send transaction to iroha
                tx_processor_->transactionHandle(tx);

-                this->addTxToCacheAndLog(
+                this->pushStatus(


Here and above there is a slight duplication, maybe put it in a function?

nickaleks · 2018-07-23T13:24:47Z

test/module/irohad/torii/processor/transaction_processor_test.cpp

-    status_map[response->transactionHash()] = response;
-  });
+  EXPECT_CALL(*status_bus, publish(_))
+      // evey transaction is notified that it is first


nickaleks · 2018-07-24T08:02:46Z

test/framework/integration_framework/integration_test_framework.cpp

@@ -19,6 +19,7 @@

 #include <memory>

+#include <boost/thread/barrier.hpp>


Add whitespace please

muratovv · 2018-07-24T10:05:01Z

irohad/main/application.cpp

-
-  command_service = std::make_shared<::torii::CommandService>(
-      tx_processor, storage, std::chrono::seconds(1), 2 * proposal_delay_);
+  status_bus_ = std::make_shared<StatusBusImpl>();


Maybe move status bus as a separate method? I think it is will be more clear to a reader of the code. And, also, we will introduce status bus as a dependency in other modules.

muratovv · 2018-07-24T10:09:28Z

irohad/torii/impl/command_service.cpp

@@ -69,6 +64,16 @@ namespace torii {
    });
  }

+  namespace {
+    iroha::protocol::ToriiResponse makeResponse(
+        shared_model::crypto::Hash h, iroha::protocol::TxStatus status) {


What is the point to pass parameters as value instead of const ref?

muratovv · 2018-07-24T10:10:28Z

irohad/torii/impl/command_service.cpp

@@ -69,6 +64,16 @@ namespace torii {
    });
  }

+  namespace {
+    iroha::protocol::ToriiResponse makeResponse(


Add documentation for the method.

Isn't it self-explanatory?

But the purpose is hidden.

anonymous namespace make a clue that it's for internal usage
And there's nothing fancy, it just make response with the passed params

muratovv · 2018-07-24T10:11:40Z

irohad/torii/impl/command_service.cpp

+  namespace {
+    iroha::protocol::ToriiResponse makeResponse(
+        shared_model::crypto::Hash h, iroha::protocol::TxStatus status) {
+      iroha::protocol::ToriiResponse response;


Why are we using raw proto instead of the model?

Because it used for grpc

muratovv · 2018-07-24T10:14:18Z

irohad/torii/impl/command_service.cpp

@@ -270,7 +264,8 @@ namespace torii {
                  .build()
                  .getTransport();
            }())));
-    return responses_
+    return status_bus_
+        ->statuses()


Is this code is well-formed with clang-format?

muratovv · 2018-07-24T10:23:18Z

irohad/torii/status_bus.hpp

+     public:
+      virtual ~StatusBus() = default;
+
+      using Objects =


Add doc for using

muratovv · 2018-07-24T10:24:04Z

irohad/torii/status_bus.hpp

+      /**
+       * Shares object among the bus users
+       * @param object to share
+       */


Add guarantees of interface about multi-thread interaction. This documentation may be moved to impl class doc.

muratovv · 2018-07-24T10:25:22Z

irohad/torii/status_bus.hpp

+      /**
+       * @return observable over objects in bus
+       */
+      virtual rxcpp::observable<Objects> statuses() = 0;


Add information about the thread where subscribes will be invoked. This documentation may be moved to impl class doc.

muratovv · 2018-07-24T10:26:41Z

irohad/torii/status_bus.hpp

+#include "interfaces/transaction_responses/tx_response.hpp"
+
+namespace iroha {
+  namespace torii {


Why is the namespace torii? Mabe move it class to libs?

muratovv · 2018-07-24T10:29:46Z

test/framework/integration_framework/integration_test_framework.cpp

@@ -179,10 +180,29 @@ namespace integration_framework {
      std::function<void(const shared_model::proto::TransactionResponse &)>
          validation) {
    log_->info("send transaction");
+
+    boost::barrier bar1(2);


Add comment with explanation of barriers here.

- Add StatusBusImpl - Add MockStatusBus - Introduce boost::thread - Use proper synchronization for recieving stateless statuses in ITF::sendTx - Fix cache issue in CommandService - Update dockerfiles - Refactor CommandService with makeResponse Signed-off-by: Kitsu <mail@kitsu.me>

muratovv · 2018-07-26T09:00:25Z

irohad/torii/impl/status_bus_impl.cpp

+namespace iroha {
+  namespace torii {
+    StatusBusImpl::StatusBusImpl(rxcpp::observe_on_one_worker worker)
+        : worker_(worker), subject_(worker_) {}


What is the point to hold worker_ field?

If it passed directly to the subject_ that will create a new thread for each subscriber

Signed-off-by: Kitsu <mail@kitsu.me>

dram · 2018-08-21T04:32:50Z

As boost::thread dependency is introduced, I think that docs related to boost installation also need to be updated, i.e. https://github.com/hyperledger/iroha/blame/v1.0.0_beta-4/docs/source/guides/build.rst#L78-L82

l4l added the needs-review pr awaits review from maintainers label Jul 18, 2018

l4l requested review from nickaleks, lebdron and muratovv July 18, 2018 22:34

nickaleks suggested changes Jul 20, 2018

View reviewed changes

l4l requested a review from nickaleks July 23, 2018 08:43

nickaleks suggested changes Jul 23, 2018

View reviewed changes

nickaleks approved these changes Jul 24, 2018

View reviewed changes

muratovv suggested changes Jul 24, 2018

View reviewed changes

l4l force-pushed the develop branch 8 times, most recently from 4750870 to b2a5906 Compare July 25, 2018 17:15

l4l force-pushed the feature/tx_status_bus branch from c7772f9 to 9b40cb0 Compare July 26, 2018 08:29

muratovv approved these changes Jul 26, 2018

View reviewed changes

Add comment about worker_ member purpose

68f2238

Signed-off-by: Kitsu <mail@kitsu.me>

l4l removed the needs-review pr awaits review from maintainers label Jul 26, 2018

l4l merged commit 19f3cd6 into develop Jul 26, 2018

l4l deleted the feature/tx_status_bus branch July 26, 2018 10:35

neewy mentioned this pull request Jul 26, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l mentioned this pull request Aug 27, 2018

Add thread component in boost bootstrapping #1680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/tx status bus #1575

Feature/tx status bus #1575

l4l commented Jul 18, 2018 •

edited

Loading

nickaleks Jul 20, 2018

nickaleks Jul 20, 2018

nickaleks Jul 20, 2018

nickaleks Jul 20, 2018

nickaleks Jul 20, 2018

nickaleks Jul 23, 2018

nickaleks Jul 23, 2018

nickaleks Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 24, 2018

l4l Jul 24, 2018

muratovv Jul 24, 2018

l4l Jul 24, 2018

muratovv Jul 24, 2018

l4l Jul 24, 2018

muratovv Jul 24, 2018

l4l Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 24, 2018

muratovv Jul 26, 2018

l4l Jul 26, 2018

dram commented Aug 21, 2018

		@@ -19,6 +19,7 @@

		#include <memory>

		#include <boost/thread/barrier.hpp>

Feature/tx status bus #1575

Feature/tx status bus #1575

Conversation

l4l commented Jul 18, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

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

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

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

dram commented Aug 21, 2018

l4l commented Jul 18, 2018 •

edited

Loading