Opened 7 years ago

Closed 7 years ago

#2935 closed defect (fixed)

the "checkin" callback for asiodns server classes should be removed

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20130806
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0.93 Internal?: no

Description

The "checkin" callbacks passed to asiodns::UDP/TCPServer class
are meaningless. I don't know why they were introduced, but at least
currently the only purpose of them is to check whether any incoming
command (including config update) is delivered to a CC channel.

First, this is meaningless, because ModuleCCSession watches the
channel (on top of the same asio::io_service) asynchronously itself.
Secondly, it's not effective because if that were the only way to get
commands, an idle server (one that is not receiving queries) could never
get commands. Third, it's performance bottleneck because this is
basically a polling invoked for every incoming query.

For these reasons, I'm going to remove it from the SyncUDPServer
class in #2903. The performance reason is the strongest reason
because that's the main reason why we introduce SyncUDPServer in the
first place. For other classes, it does not do any harm other than
being a useless bottleneck, but when we have time it's better to clean
it up if only for clarity.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 3

comment:2 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:3 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130806

comment:4 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:5 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

After updating libb10-asiodns to remove the checkin callback completely, I noticed that libb10-resolve requires this callback for its testing. See recursive_query_unittest.cc, etc. It seems we cannot remove this callback.

The partial work has been pushed to trac2935. Please advise in review.

comment:6 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

It seems to me the tests only need a callback, but it is not crucial for the callback to be the checkin one. As the other two callbacks are NULL, we can use the lookup one, for example. The only issue is it provides more (unneeded) parameters, but that's not big deal.

The commit looks sane from the look at it.

comment:8 Changed 7 years ago by muks

  • Owner changed from muks to vorner

Ok changes done. Back to review.

comment:9 Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Just a question, the ConfigCheck class in resolver is still used by something? Can't that one be removed? And similarly for the auth server?

comment:10 Changed 7 years ago by muks

  • Owner changed from muks to vorner

These have been removed. The functionality inside operator() in ConfigCheck seems to be handled inside CC session itself now. ModuleCCSession::start() adds startCheck() and handles checkCommand() there.

make check and lettuce pass here.

comment:11 Changed 7 years ago by vorner

  • Owner changed from vorner to muks
  • Total Hours changed from 0 to 0.93

I think it is ready for merge now.

comment:12 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit 2b38d4d369e3519b835cda94f1f167b4b96d5a0f:

* b920504 [2935] Remove unused classes
* 434edbb [2935] Cleanup checkin callback from b10-resolver
* 9b71351 [2935] Cleanup checkin callback from b10-auth
* c46e167 [2935] Remove use of checkin callback in libb10-resolve
* 2d47b14 [2935] Remove the "checkin" callback for asiodns server classes

Resolving as fixed. Thank you for the reviews Michal.

Note: See TracTickets for help on using tickets.