Merge lp:~stewart/galera/bug907623 into lp:galera/2.x

Proposed by Stewart Smith on 2012-01-04
Status: Merged
Approved by: Alex Yurchenko on 2012-01-06
Approved revision: 109
Merge reported by: Alex Yurchenko
Merged at revision: not available
Proposed branch: lp:~stewart/galera/bug907623
Merge into: lp:galera/2.x
Diff against target: 187 lines (+50/-15)
3 files modified
galerautils/src/gu_network.cpp (+14/-1)
galerautils/tests/gu_net_test.cpp (+34/-13)
gcomm/test/check_util.cpp (+2/-1)
To merge this branch: bzr merge lp:~stewart/galera/bug907623
Reviewer Review Type Date Requested Status
Alex Yurchenko 2012-01-04 Approve on 2012-01-09
Review via email: mp+87434@code.launchpad.net

Description of the change

This branch enables the test suite to be run multiple times concurrently on the same machine.

Prior to this branch being merged, if you simply built Galera by using "scons" or build script in two completely different directories on the same machine, you could get test failures due to the test suite trying to open the same TCP port.

This branch changes the test suite to instead of having hard coded port numbers, to use Operating System assigned ones. This means you can have as many concurrent builds on a single machine as you have free TCP ports.

You get an OS assigned port number simply by binding to port 0 instead of 10001 or some such number. You then just ask the OS for what port number it gave you.

The changes are pretty minor to get there, and it *really* helps when you're running Continuous Integration.

To post a comment you must log in.
Alex Yurchenko (ayurchen) wrote :

gu::Network is deprecated in favour of asio and should be removed from the code.

review: Disapprove
Stewart Smith (stewart) wrote :

I do not think that is a valid reason to reject this. This patch is a simple fix for a current problem, it is not a refactor of existing code (and that should be a separate patch). Rewriting large swaths of code just so that you can compile simultaneously doesn't really seem like the right way to do it - and certainly makes it harder to test any refactored code.

I really hope that you reconsider this one, as without a patch like this it makes it really hard for us to do further build and test.

Alex Yurchenko (ayurchen) wrote :

I have disabled gu::Network unit tests, so they should not interfer with the build anymore. Is there still a problem?

If you find that disabling unit tests is not enough and you still wish us to incorporate this patch, please explicitly assign the copyright to Codership Oy. (Which I'm not sure how to do with LP, honestly. Perhaps in the comments?)

review: Resubmit
Stewart Smith (stewart) wrote :

It looks like check_util is still run, which also deals with a TCP port. So it probably is a good idea to merge this branch.

I'm not sure what you need for copyright assignment - but I hereby assign Codership Oy all rights to the code in this merge request.

Alex Yurchenko (ayurchen) wrote :

Yes, our bad. We merged the relevant part of the patch in revno 107, please try if that works.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'galerautils/src/gu_network.cpp'
2--- galerautils/src/gu_network.cpp 2011-02-01 19:13:25 +0000
3+++ galerautils/src/gu_network.cpp 2012-01-04 05:59:23 +0000
4@@ -441,8 +441,21 @@
5 }
6
7 set_opt(this, ai, ::get_opt(uri));
8- listener_ai = new Addrinfo(ai);
9+
10+ Sockaddr local_sa(sa);
11+ struct sockaddr *servaddr= &(local_sa.get_sockaddr());
12+ socklen_t sock_len= local_sa.get_sockaddr_len();
13+
14+ if (::getsockname(fd, servaddr, &sock_len) == -1)
15+ {
16+ set_state(S_FAILED, errno);
17+ gu_throw_error(errno);
18+ }
19+
20+ listener_ai = new Addrinfo(ai, local_sa);
21 set_state(S_LISTENING);
22+
23+ local_addr = listener_ai->to_string();
24 }
25
26 gu::net::Socket* gu::net::Socket::accept()
27
28=== modified file 'galerautils/tests/gu_net_test.cpp'
29--- galerautils/tests/gu_net_test.cpp 2011-02-01 19:13:25 +0000
30+++ galerautils/tests/gu_net_test.cpp 2012-01-04 05:59:23 +0000
31@@ -197,7 +197,7 @@
32 {
33 log_info << "START";
34 Network net;
35- Socket* listener = net.listen("tcp://localhost:2112");
36+ Socket* listener = net.listen("tcp://localhost:0");
37 listener->close();
38 delete listener;
39 }
40@@ -301,16 +301,19 @@
41 gu_log_max_level = GU_LOG_DEBUG;
42 log_info << "START";
43 Network* net = new Network;
44- Socket* listener = net->listen("tcp://localhost:2112");
45+ Socket* listener = net->listen("tcp://localhost:0");
46
47 log_info << "listener " << listener->get_local_addr();
48
49+ std::string uri_str;
50+ uri_str= listener->get_local_addr();
51+
52 listener_thd_args args = {net, 2, 0, 0};
53 pthread_t th;
54 pthread_create(&th, 0, &listener_thd, &args);
55
56 Network* net2 = new Network;
57- Socket* conn = net2->connect("tcp://localhost:2112");
58+ Socket* conn = net2->connect(uri_str);
59
60 fail_unless(conn != 0);
61 fail_unless(conn->get_state() == Socket::S_CONNECTED);
62@@ -318,7 +321,7 @@
63 log_info << "connected remote " << conn->get_remote_addr();
64 log_info << "connected local " << conn->get_local_addr();
65
66- Socket* conn2 = net2->connect("tcp://localhost:2112");
67+ Socket* conn2 = net2->connect(uri_str);
68 fail_unless(conn2 != 0);
69 fail_unless(conn2->get_state() == Socket::S_CONNECTED);
70
71@@ -357,18 +360,21 @@
72 }
73
74 Network* net = new Network;
75- Socket* listener = net->listen("tcp://localhost:2112");
76+ Socket* listener = net->listen("tcp://localhost:0");
77+
78+ std::string uri_str(listener->get_local_addr());
79+
80 listener_thd_args args = {net, 2, buf, bufsize};
81 pthread_t th;
82 pthread_create(&th, 0, &listener_thd, &args);
83
84 Network* net2 = new Network;
85- Socket* conn = net2->connect("tcp://localhost:2112");
86+ Socket* conn = net2->connect(uri_str);
87
88 fail_unless(conn != 0);
89 fail_unless(conn->get_state() == Socket::S_CONNECTED);
90
91- Socket* conn2 = net2->connect("tcp://localhost:2112");
92+ Socket* conn2 = net2->connect(uri_str);
93 fail_unless(conn2 != 0);
94 fail_unless(conn2->get_state() == Socket::S_CONNECTED);
95
96@@ -486,7 +492,8 @@
97 }
98 END_TEST
99
100-static void make_connections(Network& net,
101+static void make_connections(std::string uri_str,
102+ Network& net,
103 vector<Socket*>& cl,
104 vector<Socket*>& sr,
105 size_t n)
106@@ -495,7 +502,7 @@
107 sr.resize(n);
108 for (size_t i = 0; i < n; ++i)
109 {
110- cl[i] = net.connect("tcp://localhost:2112?socket.non_blocking=1");
111+ cl[i] = net.connect(uri_str);
112 }
113 size_t sr_cnt = 0;
114 size_t cl_cnt = 0;
115@@ -555,12 +562,16 @@
116 log_info << "START";
117 Network net;
118
119- Socket* listener = net.listen("tcp://localhost:2112?socket.non_blocking=1");
120+ Socket* listener = net.listen("tcp://localhost:0?socket.non_blocking=1");
121+ std::string uri_str(listener->get_local_addr());
122+
123+ uri_str.append("?socket.non_blocking=1");
124+
125 log_info << "listener: " << *listener;
126 vector<Socket*> cl;
127 vector<Socket*> sr;
128 gu_log_max_level = GU_LOG_DEBUG;
129- make_connections(net, cl, sr, 3);
130+ make_connections(uri_str, net, cl, sr, 3);
131
132 close_connections(net, cl, sr);
133
134@@ -666,6 +677,8 @@
135
136 public:
137
138+ std::string get_url() { return listener->get_local_addr(); }
139+
140 void connect(const string& url)
141 {
142 send_sock = net.connect(url);
143@@ -782,8 +795,12 @@
144 START_TEST(test_net_consumer)
145 {
146 log_info << "START";
147- string url("tcp://localhost:2112?socket.non_blocking=1");
148+ string url("tcp://localhost:0?socket.non_blocking=1");
149 NetConsumer cons(url);
150+
151+ url= cons.get_url();
152+ url.append("?socket.non_blocking=1");
153+
154 cons.connect(url);
155
156 cons.start();
157@@ -853,8 +870,12 @@
158 START_TEST(test_net_consumer_nto1)
159 {
160 log_info << "START";
161- string url("tcp://localhost:2112?socket.non_blocking=1");
162+ string url("tcp://localhost:0?socket.non_blocking=1");
163 NetConsumer cons(url);
164+
165+ url= cons.get_url();
166+ url.append("?socket.non_blocking=1");
167+
168 cons.connect(url);
169
170 cons.start();
171
172=== modified file 'gcomm/test/check_util.cpp'
173--- gcomm/test/check_util.cpp 2011-02-01 19:13:25 +0000
174+++ gcomm/test/check_util.cpp 2012-01-04 05:59:23 +0000
175@@ -58,10 +58,11 @@
176 {
177 gu::Config conf;
178 AsioProtonet pn(conf);
179- const string uri_str("tcp://localhost:10001");
180+ string uri_str("tcp://localhost:0");
181
182 Acceptor* acc = pn.acceptor(uri_str);
183 acc->listen(uri_str);
184+ uri_str = acc->listen_addr();
185
186 SocketPtr cl = pn.socket(uri_str);
187 cl->connect(uri_str);

Subscribers

People subscribed via source and target branches

to all changes: