Opened 8 years ago

Closed 8 years ago

#1820 closed defect (fixed)

remove DNSService::addServer()

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

Description

Since we switched to FD-based creation we don't use these methods
in our system any more. Keeping such effectively dead code increases
maintenance cost and decreases readability, so I propose removing
them.

From a quick look, one of the constructors and some other methods in
the implementation class can (and should) also be removed.

(Even if we have some expectation that we may reuse them in an
unforeseen future, we could do that any time when the plan is actually
realized by retrieving the old code from the repository)

See also the related discussion in #1784.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by jinmei

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120403
  • Owner set to jinmei
  • Status changed from new to accepted

(moving to the current sprint. this seems to be an easy cleanup)

comment:2 Changed 8 years ago by jinmei

trac1820 is ready for review.

There was actually one other place that used the mostly-unused
interfaces of DNSService. But it was only in unit tests, so I thought
it still makes sense to clean it up and keep the public code simpler.

While doing the cleanup I noticed many unsafe places in
recursive_query_unittest (in terms of resource leak with exceptions),
so I also made them safer. This part is irrelevant to the subject of
this task, but the branch is small so I think it's acceptable.

While this branch modifies a publicly visible API, I don't think it
worth a changelog entry because it's mostly for internal-only part of
the API.

comment:3 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:4 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:5 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

The code looks OK, but I have two questions nevertheless:

  • I think I saw the ScopedSocket somewhere already. Wouldn't it be possible to unify the occurrences?
  • The methods of the DNSService were removed. Should the corresponding constructors of the UDPServer, UDPSyncServer and TCPServer be removed as well in this ticket?

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

Replying to vorner:

Thanks for the review.

The code looks OK, but I have two questions nevertheless:

  • I think I saw the ScopedSocket somewhere already. Wouldn't it be possible to unify the occurrences?

I think it's possible, and I actually thought about that while I
worked on this ticket. That should be out of scope of this ticket,
though - shall I create a ticket?

  • The methods of the DNSService were removed. Should the corresponding constructors of the UDPServer, UDPSyncServer and TCPServer be removed as well in this ticket?

Good point, I think they should, and done.

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Replying to jinmei:

The code looks OK, but I have two questions nevertheless:

  • I think I saw the ScopedSocket somewhere already. Wouldn't it be possible to unify the occurrences?

I think it's possible, and I actually thought about that while I
worked on this ticket. That should be out of scope of this ticket,
though - shall I create a ticket?

Yes, please. And I'd prefer it to be in next-sprint-proposed if possible, I kind of consider duplicate code a bug (or at least an bug egg, only waiting to hatch to a real functionality bug).

  • The methods of the DNSService were removed. Should the corresponding constructors of the UDPServer, UDPSyncServer and TCPServer be removed as well in this ticket?

Good point, I think they should, and done.

Thanks. Please merge.

comment:9 Changed 8 years ago by vorner

  • Total Hours changed from 0 to 1.01

Why do I always forget to update the time when saying „OK to merge“?

comment:10 in reply to: ↑ 8 Changed 8 years ago by jinmei

Replying to vorner:

Replying to jinmei:

The code looks OK, but I have two questions nevertheless:

  • I think I saw the ScopedSocket somewhere already. Wouldn't it be possible to unify the occurrences?

I think it's possible, and I actually thought about that while I
worked on this ticket. That should be out of scope of this ticket,
though - shall I create a ticket?

Yes, please. And I'd prefer it to be in next-sprint-proposed if possible, I kind of consider duplicate code a bug (or at least an bug egg, only waiting to hatch to a real functionality bug).

Created (#1879).

  • The methods of the DNSService were removed. Should the corresponding constructors of the UDPServer, UDPSyncServer and TCPServer be removed as well in this ticket?

Good point, I think they should, and done.

Thanks. Please merge.

Okay, thanks. Merge done, closing.

comment:11 Changed 8 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.01 to 5.33
Note: See TracTickets for help on using tickets.