Mailing-List: contact dev-help@qpid.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@qpid.apache.org
Content-Type: multipart/alternative;
 boundary="===============6668797240738523723=="
MIME-Version: 1.0
Subject: Re: Review Request 16577: QPID-5428: Heartbeats not in use when
 attempting
 to connect with python client.
From: "Andrew Stitcher" <astitcher@apache.org>
To: "Rafael Schloming" <rhs@apache.org>, "Gordon Sim" <gsim@redhat.com>
Cc: "Andrew Stitcher" <astitcher@apache.org>,
 "Alan Conway" <aconway@redhat.com>, "qpid" <dev@qpid.apache.org>
Date: Tue, 07 Jan 2014 22:10:05 -0000
Message-ID: <20140107221005.1151.62946@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Andrew Stitcher" <noreply@reviews.apache.org>
References: <20140103002244.26158.67096@reviews.apache.org>
In-Reply-To: <20140103002244.26158.67096@reviews.apache.org>
Reply-To: "Andrew Stitcher" <astitcher@apache.org>

--===============6668797240738523723==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


> On Jan. 3, 2014, 12:22 a.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/tests/ha_tests.py, line 237
> > <https://reviews.apache.org/r/16577/diff/1/?file=413695#file413695line237>
> >
> >     This doesn't appear to be an HA test why is it in these tests?
> 
> Alan Conway wrote:
>     Agreed, should be in the qpid/python tests somewhere. I'll look for an appropriate place.
> 
> Alan Conway wrote:
>     The trick here is that the failover part of the test requires starting and killing a second broker, which is beyond the capabilities of the qpid/python tests.
>     Options I can see:
>     - leave it where it is since it tests failover which is HA-related
>     - create a new cpp/src/tests/python_client_test.py test suite (using brokertest.py) with this test to start, can collect further python client tests there that need to start/stop brokers but don't require the use of the HA plugin.
>     I will do the latter if you think it is worthwhile.

Hmm, I think the broker starting functionality in the python test framework is much more recent than the original python tests, but perhaps the original python tests should actually start the broker itself.

On the other hand I think the python test can actually be used against both the c++ and java brokers, so perhaps it should only start the broker if how to do that can be specified.

If this test is specific to the C++ broker then I think it probably should actually start a new set of tests and we should migrate tests there that are specific to the C++ broker.

I'll stop rambling now - hope that made sense.


> On Jan. 3, 2014, 12:22 a.m., Andrew Stitcher wrote:
> > /trunk/qpid/cpp/src/tests/ha_tests.py, line 243
> > <https://reviews.apache.org/r/16577/diff/1/?file=413695#file413695line243>
> >
> >     I don't see anywhere that you send SIGCONT to the broker to get it to carry on - how do subsequent tests work?
> 
> Alan Conway wrote:
>     The brokertest framework automatically tears down processes (including brokers) started by each test, each test starts new brokers as needed.

That sounds pretty inefficient - do you really mean that the broker gets torn down for every test rather than say every test suite?

If so that would make these tests much slower than perhaps necessary.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16577/#review31093
-----------------------------------------------------------


On Jan. 6, 2014, 8:46 p.m., Alan Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16577/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2014, 8:46 p.m.)
> 
> 
> Review request for qpid, Gordon Sim and Rafael Schloming.
> 
> 
> Bugs: qpid-5428
>     https://issues.apache.org/jira/browse/qpid-5428
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> QPID-5428: Heartbeats not in use when attempting to connect with python client.
> 
> Heartbeats ignored when opening a connection, could hang indefinitely
> Need to cover 3 cases (test included):
> - Connect sucessful but then broker stalls.
> - Connect to a stalled broker that never responds.
> - Fail-over to a stalled broker that never responds
> 
> All cases are handled by the following fixes to driver.py:
> - Check for heartbeats even before engine._connected since we may time out
>   before receiving open-ok if the peer is stalled and never sends data.
> - Set _last_in and _last_out so that we time heartbeats from the start of the
>   connection if no data is ever sent or received.
> - Call self.update_status in Driver.timeout to detect connection closed due to
>   heartbeat timeout (rather than a readable or writeable event.)
>   Make update_status a no-op if engine or transport are not yet set up.
> - Don't consider reconnect complete in connect(), wait till we get the open-ok.
>   See the comment on Driver._check_retry_ok()
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/tests/ha_tests.py 1555989 
>   /trunk/qpid/python/qpid/messaging/driver.py 1555989 
> 
> Diff: https://reviews.apache.org/r/16577/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alan Conway
> 
>


--===============6668797240738523723==--