Update: update network interface. by ifplusor · Pull Request #60 · apache/rocketmq-client-cpp

ifplusor · 2019-01-08T11:01:19Z

feature: use only one event loop for all TcpTransport.

src/transport/EventLoop.cpp

src/transport/TcpTransport.cpp

lizhanhui · 2019-01-11T02:21:12Z

This pull request brings huge improvement to the thread model. The current implementation is ugly and inefficient. Thanks @ifplusor

stcai

Nice job, but there are some little problems need to fix.

src/transport/TcpRemotingClient.h

stcai · 2019-01-13T15:46:59Z

src/transport/TcpRemotingClient.cpp

+  LOG_INFO("TcpRemotingClient::boost asio async service running");
+
+#if !defined(WIN32) && !defined(__APPLE__)
+  prctl(PR_SET_NAME, "RemotingAsioT", 0, 0, 0);


The thread name looks weired. Could you please name a regular one?

suffix 'T' is thread just as 'TP' is threadpool. and i have no idea what is fitting

src/transport/TcpRemotingClient.cpp

ShannonDing · 2019-01-21T09:28:44Z

This is a big improvement, it is better to verify it more carefully and then release in the next version.

ifplusor · 2019-01-21T09:35:14Z

@ShannonDing you are right. i am tracing another infinite loop issue, it block in rebalance. and i am also testing this in my beta environment.

jovany-wang

Great! I left some comments.

src/transport/EventLoop.cpp

jovany-wang

I think we should add more detail docstring for the new file like eventloop.

Also, I have not finished reviewing this such huge PR yet.

jovany-wang · 2019-02-17T14:00:10Z

src/common/AsyncCallbackWrap.h

-                                 bool bProducePullRequest);
-  virtual void onException();
-  virtual asyncCallBackType getCallbackType() { return sendCallbackWrap; }
+  ~SendCallbackWrap() override = default;;


I think it's unnecessary to remove virtual here though it's an override.

clion prompt to use override and remove virtual.

jovany-wang · 2019-02-17T14:00:15Z

src/common/AsyncCallbackWrap.h

 public:
-  SendCallbackWrap(const string& brokerName, const MQMessage& msg,
-                   AsyncCallback* pAsyncCallback, MQClientAPIImpl* pclientAPI);
+  SendCallbackWrap(const string &brokerName, const MQMessage &msg,


remove using namespace std; and use std::string here.

Nice catch, best to avoid namespace pollution.

using namespace std;

in another head file.

src/common/AsyncCallbackWrap.h

src/common/AsyncCallbackWrap.cpp

src/transport/EventLoop.h

src/transport/ResponseFuture.h

jovany-wang · 2019-02-17T14:44:09Z

src/transport/ResponseFuture.h

  int m_requestCode;
  int m_opaque;
-  bool m_sendRequestOK;
+  int64 m_timeout;  // ms


int64 m_timeout_ms;

src/transport/ResponseFuture.h

jovany-wang · 2019-02-17T14:45:59Z

src/transport/TcpRemotingClient.cpp

+#endif
+
+  // avoid async io service stops after first timer timeout callback
+  boost::asio::io_service::work work(m_async_ioService);


rename m_async_ioService to m_async_io_service

I don't rename this variable.

messense · 2019-04-25T01:56:11Z

Any progress on getting this merged?

ifplusor · 2019-04-25T02:04:18Z

Any progress on getting this merged?

@messense this PR is so large, and review is hard. i think we will merge it in next version, and i have more commits that are dependent on this.

- feature: use only one event loop for all TcpTransport. - update: network components.

…Client, TcpTransport and ReponseFunture.

* update network interface. - feature: use only one event loop for all TcpTransport. - update: network components. * remove boost mutex, timed_mutex and condition_variable in TcpRemotingClient, TcpTransport and ReponseFunture.

* [ISSUE apache#142] save string::find result into a string::size_type variable. declare correct string::size_type by auto. (apache#143) * batch issue * Update: update network interface. (apache#60) * update network interface. - feature: use only one event loop for all TcpTransport. - update: network components. * remove boost mutex, timed_mutex and condition_variable in TcpRemotingClient, TcpTransport and ReponseFunture.

ShannonDing mentioned this pull request Jan 8, 2019

[ISSUE #52]Fix infinite loop on tcptransport connect #59

Merged

ifplusor force-pushed the feature-eventloop branch from 6b7dc43 to ca755a0 Compare January 9, 2019 03:01

ShannonDing added bug Something isn't working enhancement New feature or request labels Jan 9, 2019

ShannonDing added this to the 1.2.1 milestone Jan 9, 2019

ifplusor mentioned this pull request Jan 10, 2019

Get Transport death lock if all brokers shutdown unnormally #52

Closed

lizhanhui reviewed Jan 11, 2019

View reviewed changes

src/transport/EventLoop.cpp Show resolved Hide resolved

lizhanhui reviewed Jan 11, 2019

View reviewed changes

src/transport/TcpTransport.cpp Outdated Show resolved Hide resolved

ifplusor force-pushed the feature-eventloop branch 2 times, most recently from ec579ce to 7873156 Compare January 11, 2019 19:17

stcai suggested changes Jan 13, 2019

View reviewed changes

ifplusor force-pushed the feature-eventloop branch 6 times, most recently from 902b53b to 125c84e Compare January 18, 2019 09:22

ifplusor changed the title ~~feature: only one event loop for all TcpTransport.~~ Update: update network interface. Jan 18, 2019

ShannonDing modified the milestones: 1.2.1, 1.2.2 Jan 21, 2019

ifplusor force-pushed the feature-eventloop branch 7 times, most recently from 29848f9 to 908517c Compare January 28, 2019 08:17

ifplusor force-pushed the feature-eventloop branch 2 times, most recently from be1a165 to 1228032 Compare January 30, 2019 14:38

jovany-wang suggested changes Feb 15, 2019

View reviewed changes

src/transport/EventLoop.cpp Outdated Show resolved Hide resolved

src/transport/EventLoop.cpp Outdated Show resolved Hide resolved

src/transport/EventLoop.cpp Outdated Show resolved Hide resolved

ifplusor force-pushed the feature-eventloop branch from 1228032 to d1e19dd Compare February 17, 2019 01:41

jovany-wang suggested changes Feb 17, 2019

View reviewed changes

ifplusor force-pushed the feature-eventloop branch 2 times, most recently from 5f224bd to a164e06 Compare February 18, 2019 09:08

ifplusor force-pushed the feature-eventloop branch from ad18df4 to 4e5a8cd Compare March 4, 2019 02:19

ifplusor force-pushed the feature-eventloop branch 3 times, most recently from 0996785 to 358546a Compare March 15, 2019 10:19

ifplusor force-pushed the feature-eventloop branch 2 times, most recently from a8b2970 to 122cd44 Compare April 3, 2019 11:25

ifplusor force-pushed the feature-eventloop branch 2 times, most recently from 1e00c4e to b3f1c5a Compare April 18, 2019 07:29

ShannonDing modified the milestones: 1.2.2, 1.2.3 Apr 19, 2019

ifplusor force-pushed the feature-eventloop branch 2 times, most recently from f5321b2 to eefe849 Compare April 29, 2019 09:39

ifplusor added 2 commits April 30, 2019 09:56

update network interface.

62176ab

- feature: use only one event loop for all TcpTransport. - update: network components.

remove boost mutex, timed_mutex and condition_variable in TcpRemoting…

abb79a4

…Client, TcpTransport and ReponseFunture.

ifplusor force-pushed the feature-eventloop branch from eefe849 to abb79a4 Compare April 30, 2019 01:59

ShannonDing merged commit 5689607 into apache:master Jun 12, 2019

ifplusor deleted the feature-eventloop branch June 14, 2019 05:37

ifplusor mentioned this pull request Aug 26, 2019

connect to broker too much time #132

Closed

Conversation

ifplusor commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lizhanhui commented Jan 11, 2019

Uh oh!

stcai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ShannonDing commented Jan 21, 2019

Uh oh!

ifplusor commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jovany-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jovany-wang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ifplusor Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

messense commented Apr 25, 2019

Uh oh!

ifplusor commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ifplusor commented Jan 8, 2019 •

edited

Loading

ifplusor commented Jan 21, 2019 •

edited

Loading

jovany-wang left a comment •

edited

Loading

ifplusor Feb 18, 2019 •

edited

Loading

ifplusor commented Apr 25, 2019 •

edited

Loading