EPICS Base

Code review comment for lp:~ralph-lange/epics-base/ca-over-tcp

ca-over-tcp
Merge into 3.14

Revision history for this message

Ralph Lange (ralph-lange) wrote on 2010-05-14:

Thanks, Jeff.

I did some testing with a setup of two soft IOCs and the example name
server (that's actually how I found the two issues that were not working
in the 2008 Codeathon version), focussing on how the new feature behaves
in different situations ( IOCs and/or name servers going away and coming
back). I was also running the integration tests all ways (old client -
new server, new client - old server, new client - new server) to make
sure it did't obviously break existing behavior. Admittedly I did not
add any new tests to the test suite, as I found it hard to think of a
reasonable test. Do you have an idea?

If your IMHO remark refers to the bug fixes in the reference manual:
first I was reluctant, but then decided as these were fixing bugs in the
documentation only, it was not worth creating a new branch for that.

If you refer to adding a new feature to the 3.14 series: as I told you,
Andrew wants to discuss at the Codeathon which release this branch
should be merged to. Note that to escape the large number of meaningless
"sync" commit comments in the main CVS trunk, we decided a few months
ago that 3.15 should be developed from forking off the existing 3.14
branch (that has meaningful commit messages). To make this possible,
3.15 only changes in the CVS trunk have to be pulled out into
feature-branches against the 3.14 release - which is exactly what I was
doing with the "ca-over-tcp" feature. Now we can decide if that branch
should be part of 3.14 (before the split) or 3.15 (after the split).

I removed the comment in udpiiu.cpp as you suggested and pushed the
change onto LP.

Cheers,
Ralph

On Fri 14 May 2010 1:34:29 Jeff Hill wrote:
> Review: Approve
>
> Looks fine, but its hard to catch everything with a source code review so testing is important. IMHO its important to keep bug fix changes and new features in different release series, and we seem to be slipping a bit on that level of discipline.
>
> udpiiu.cpp comment on line 278 prob no-longer applies and can be removed
>

Thanks, Jeff.

I did some testing with a setup of two soft IOCs and the example name 
server (that's actually how I found the two issues that were not working 
in the 2008 Codeathon version), focussing on how the new feature behaves 
in different situations ( IOCs and/or name servers going away and coming 
back). I was also running the integration tests all ways (old client - 
new server, new client - old server, new client - new server) to make 
sure it did't obviously break existing behavior. Admittedly I did not 
add any new tests to the test suite, as I found it hard to think of a 
reasonable test. Do you have an idea?

If your IMHO remark refers to the bug fixes in the reference manual: 
first I was reluctant, but then decided as these were fixing bugs in the 
documentation only, it was not worth creating a new branch for that.

If you refer to adding a new feature to the 3.14 series: as I told you, 
Andrew wants to discuss at the Codeathon which release this branch 
should be merged to. Note that to escape the large number of meaningless 
"sync" commit comments in the main CVS trunk, we decided a few months 
ago that 3.15 should be developed from forking off the existing 3.14 
branch (that has meaningful commit messages). To make this possible, 
3.15 only changes in the CVS trunk have to be pulled out into 
feature-branches against the 3.14 release - which is exactly what I was 
doing with the "ca-over-tcp" feature. Now we can decide if that branch 
should be part of 3.14 (before the split) or 3.15 (after the split).

I removed the comment in udpiiu.cpp as you suggested and pushed the 
change onto LP.

Cheers,
Ralph

« Back to merge proposal