BIND 10 #275: refactoring CC session class for easier tests
BIND 10 Development
do-not-reply at isc.org
Fri Jul 9 18:28:08 UTC 2010
#275: refactoring CC session class for easier tests
---------------------------------------------+------------------------------
Reporter: jinmei | Owner: jinmei
Type: enhancement | Status: reviewing
Priority: major | Milestone: 06. 4th Incremental Release
Component: Inter-module communication | Resolution:
Keywords: | Sensitive: 0
Estimatedhours: | Billable: 0
Totalhours: | Internal: 0
---------------------------------------------+------------------------------
Changes (by jinmei):
* internal: => 0
* billable: => 0
Comment:
Replying to [comment:6 jelte]:
> Replying to [comment:3 jinmei]:
> >
> > I've updated the description of the constructor. But now that I've
read the comment, I wonder it would be more natural if the caller must
establish the session. As a related matter, maybe we should explicitly
detect and reject an attempt of doubly establishing a session? (I guess
the underlying ASIO implementation detects it, but we should probably
catch the invalid usage at a higher level).
> >
>
> Hmm, i think we either need to detect that establish is called
> twice, or we need to detect that it hasn't been called at all (and
> then we can choose to error or establish ourselves)
>
Let's think about it later. I suspect a near future refactoring with
a generalized ASIO link will affect the design on this, so it would
make sense to revisit this issue at that point. For the moment, I'll
just leave a comment for the description of the ModuleCCSession
constructor that the requirement is in flux. I'll also make a
separate ticket on this so that we (or trac) can remember the issue.
>
> no, i mean not putting them on the heap
>
Ah, okay.
>
> And same for ModuleCCSession. Due to the current arch this isn't
possible for auth_srv and io_service.
>
> But since we are in a main() function anyway this isn't really
important, and we can probably defer this to a refactor later.
>
As you noted, with the current architecture we can't easily make this
change, and that's the main reason why I used the new-delete pair.
Also noted in the previous comment of mine, I expect we'll improve
this part of the code in a cleaner way in a next step of refactoring.
So, for now, let's live with the current implementation.
>
> If you want to so the check-session-for-established-already here, please
do, for the rest i think this is ready for merge.
>
As noted above, I'd defer it to a separate task. I'm going to merge
the branch to trunk at this point because another important ticket
(xfr/notify) depends on this ticket.
Thanks for the review, btw:-)
--
Ticket URL: <http://bind10.isc.org/ticket/275#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list