Opened 10 years ago

Closed 10 years ago

#169 closed task (worksforme)

notify recv

Reported by: larissas Owned by: hanfeng
Priority: medium Milestone: 04. 2nd Incremental Release: Early Adopters
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

(was to be assigned to Han Feng)
(backlog item 122)
(2 week estimate)

Subtickets

Change History (7)

comment:1 Changed 10 years ago by larissas

  • Milestone set to 04. 2nd Incremental Release

comment:2 Changed 10 years ago by shane

  • Component changed from Unclassified to b10-auth
  • Owner set to hanfeng
  • Status changed from new to assigned

This is the part of the system that receives a NOTIFY packet and handles it. It should send it to the zone manager process, but this may end up being the xfrin for the next release, and then the zone manager later.

comment:3 follow-up: Changed 10 years ago by hanfeng

  • Owner changed from hanfeng to jinmei
  • Status changed from assigned to reviewing

The code is done in branch feng-authnotify, all the code resides in src/bin/auth/main.cc. The logic is that when auth server get the message, it will check whether it's notify message, if so, then send the zone name and sender's ip address to xfrin module.

The main interface for handle query is proessMessage, but the interface doesn't includes the sender's ip address, so current implementation is a quick hack, and I will do the refractor after coming release.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 10 years ago by jinmei

  • Owner changed from jinmei to hanfeng

Replying to hanfeng:

The code is done in branch feng-authnotify, all the code resides in src/bin/auth/main.cc.

Looking at the diff:
svn diff -r 1914 svn+ssh://bind10.isc.org/svn/bind10/branches/feng-authnotify
there doesn't seem to be any test in the branch. (It also means it's not developed test-driven, which is not good.)

Please at least add detailed tests first.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 10 years ago by hanfeng

  • Priority changed from major to critical

Please at least add detailed tests first.

The code is direct hacked into main.cc just like axfr logic, there is no head file for the two added function, so it's hard to add it into unit test. Actually, the axfr and ixfr logic should be handle all in handleMessage, but current interface can't support them.
I have write separate script to send package and check the reply. I will add the test code into unit test, once the refractor of auth server is done.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by jinmei

Replying to hanfeng:

Please at least add detailed tests first.

The code is direct hacked into main.cc just like axfr logic, there is no head file for the two added function, so it's hard to add it into unit test. Actually, the axfr and ixfr logic should be handle all in handleMessage, but current interface can't support them.
I have write separate script to send package and check the reply. I will add the test code into unit test, once the refractor of auth server is done.

IMO, this cannot be an excuse for skipping tests.

If the reason you cannot write tests is dependency on another refactoring, we should refactor the code first, then write tests for the new feature, and then design and code the new feature.

We can easily find an excuse for skipping tests (because they are often boring and implementing new features tends to look more interesting), and IMO there should be very high bar to allow that to fight against that temptation. We lowered the bar for the year 1 release due to a hard deadline based on contract, but we should consider it a very exceptional case, rather than a precedent.

Also, if we found some new feature may be difficult to test just a day before a code freeze, whether it's due to dependency on another task or other reasons, that would mean we underestimate the planning, either or both about dependency consideration and about necessary time for that work. We should learn from that so that we can make a better estimation next time. If we allow to skip tests (or documentation or review) for some easy reason, it would make it difficult to revisit and improve the estimation because that means "we were accurate".

Anyway, this is a more general topic, and many of the points are my personal opinion, for which others may disagree. Perhaps we can discuss it tomorrow?

Meanwhile I'll take a closer look at the branch to see how we can move forward practically. Unfortunately, however, I suspect we must give up including this feature in the next release.

comment:7 in reply to: ↑ 6 Changed 10 years ago by hanfeng

  • Priority changed from critical to major
  • Resolution set to worksforme
  • Status changed from reviewing to closed

For the testing rule, I quite agree with you. If there is a rule which is very practical and useful we should follow, therefore I won't add the feature into trunk for coming release. And I would like to do the refactoring once the ASIO part finished, and move the axfr and notify logic into it then add unit test for them.

Note: See TracTickets for help on using tickets.